[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

[Chuanqi Xu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG36de2d639eca: [NFC] [AST] Reduce the size of 
TemplateParmPosition (authored by ChuanqiXu).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123298/new/

https://reviews.llvm.org/D123298

Files:
  clang/include/clang/AST/DeclTemplate.h


Index: clang/include/clang/AST/DeclTemplate.h
===
--- clang/include/clang/AST/DeclTemplate.h
+++ clang/include/clang/AST/DeclTemplate.h
@@ -1148,23 +1148,40 @@
 /// parameters and is not part of the Decl hierarchy. Just a facility.
 class TemplateParmPosition {
 protected:
-  // FIXME: These probably don't need to be ints. int:5 for depth, int:8 for
-  // position? Maybe?
-  unsigned Depth;
-  unsigned Position;
+  enum { DepthWidth = 20, PositionWidth = 12 };
+  unsigned Depth : DepthWidth;
+  unsigned Position : PositionWidth;
 
-  TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {}
+  static constexpr unsigned MaxDepth = (1U << DepthWidth) - 1;
+  static constexpr unsigned MaxPosition = (1U << PositionWidth) - 1;
+
+  TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {
+// The input may fill maximum values to show that it is invalid.
+// Add one here to convert it to zero.
+assert((D + 1) <= MaxDepth &&
+   "The depth of template parmeter position is more than 2^20!");
+assert((P + 1) <= MaxPosition &&
+   "The position of template parmeter position is more than 2^12!");
+  }
 
 public:
   TemplateParmPosition() = delete;
 
   /// Get the nesting depth of the template parameter.
   unsigned getDepth() const { return Depth; }
-  void setDepth(unsigned D) { Depth = D; }
+  void setDepth(unsigned D) {
+assert((D + 1) <= MaxDepth &&
+   "The depth of template parmeter position is more than 2^20!");
+Depth = D;
+  }
 
   /// Get the position of the template parameter within its parameter list.
   unsigned getPosition() const { return Position; }
-  void setPosition(unsigned P) { Position = P; }
+  void setPosition(unsigned P) {
+assert((P + 1) <= MaxPosition &&
+   "The position of template parmeter position is more than 2^12!");
+Position = P;
+  }
 
   /// Get the index of the template parameter within its parameter list.
   unsigned getIndex() const { return Position; }


Index: clang/include/clang/AST/DeclTemplate.h
===
--- clang/include/clang/AST/DeclTemplate.h
+++ clang/include/clang/AST/DeclTemplate.h
@@ -1148,23 +1148,40 @@
 /// parameters and is not part of the Decl hierarchy. Just a facility.
 class TemplateParmPosition {
 protected:
-  // FIXME: These probably don't need to be ints. int:5 for depth, int:8 for
-  // position? Maybe?
-  unsigned Depth;
-  unsigned Position;
+  enum { DepthWidth = 20, PositionWidth = 12 };
+  unsigned Depth : DepthWidth;
+  unsigned Position : PositionWidth;
 
-  TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {}
+  static constexpr unsigned MaxDepth = (1U << DepthWidth) - 1;
+  static constexpr unsigned MaxPosition = (1U << PositionWidth) - 1;
+
+  TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {
+// The input may fill maximum values to show that it is invalid.
+// Add one here to convert it to zero.
+assert((D + 1) <= MaxDepth &&
+   "The depth of template parmeter position is more than 2^20!");
+assert((P + 1) <= MaxPosition &&
+   "The position of template parmeter position is more than 2^12!");
+  }
 
 public:
   TemplateParmPosition() = delete;
 
   /// Get the nesting depth of the template parameter.
   unsigned getDepth() const { return Depth; }
-  void setDepth(unsigned D) { Depth = D; }
+  void setDepth(unsigned D) {
+assert((D + 1) <= MaxDepth &&
+   "The depth of template parmeter position is more than 2^20!");
+Depth = D;
+  }
 
   /// Get the position of the template parameter within its parameter list.
   unsigned getPosition() const { return Position; }
-  void setPosition(unsigned P) { Position = P; }
+  void setPosition(unsigned P) {
+assert((P + 1) <= MaxPosition &&
+   "The position of template parmeter position is more than 2^12!");
+Position = P;
+  }
 
   /// Get the index of the template parameter within its parameter list.
   unsigned getIndex() const { return Position; }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D123298#3445131 , @aaron.ballman 
wrote:

> Still LG to me, but please watch the build bots and issues list closely for 
> any new template instantiation depth/position related issues over the next 
> short while given that we're making a best guess at the bit widths here.

Thanks! I looked at the build bots and the failure cases is about ompt. And I 
think it is unrelated to this patch though.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123298/new/

https://reviews.llvm.org/D123298

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Still LG to me, but please watch the build bots and issues list closely for any 
new template instantiation depth/position related issues over the next short 
while given that we're making a best guess at the bit widths here.




Comment at: clang/include/clang/AST/DeclTemplate.h:1156-1157
 
-  TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {}
+  static constexpr unsigned MaxDepth = (1l << DEPTH_BITWIDTH) - 1;
+  static constexpr unsigned MaxPosition = (1l << POSITION_BITWIDTH) - 1;
+

ChuanqiXu wrote:
> aaron.ballman wrote:
> > ChuanqiXu wrote:
> > > I don't find standard way to generate the maximum value for bit fields.. 
> > > So I choose to calculate it by hand. 
> > Since we're adding 1 everywhere we look at these values, would it make more 
> > sense to drop the -1 here and the +1 elsewhere?
> Maybe we're able to drop the -1 here. But I feel it breaks the semantics. So 
> I feel it is not good.
I don't insist. :-)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123298/new/

https://reviews.llvm.org/D123298

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu marked an inline comment as done.
ChuanqiXu added inline comments.



Comment at: clang/include/clang/AST/DeclTemplate.h:1156-1157
 
-  TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {}
+  static constexpr unsigned MaxDepth = (1l << DEPTH_BITWIDTH) - 1;
+  static constexpr unsigned MaxPosition = (1l << POSITION_BITWIDTH) - 1;
+

aaron.ballman wrote:
> ChuanqiXu wrote:
> > I don't find standard way to generate the maximum value for bit fields.. So 
> > I choose to calculate it by hand. 
> Since we're adding 1 everywhere we look at these values, would it make more 
> sense to drop the -1 here and the +1 elsewhere?
Maybe we're able to drop the -1 here. But I feel it breaks the semantics. So I 
feel it is not good.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123298/new/

https://reviews.llvm.org/D123298

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 422104.
ChuanqiXu added a comment.

Address comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123298/new/

https://reviews.llvm.org/D123298

Files:
  clang/include/clang/AST/DeclTemplate.h


Index: clang/include/clang/AST/DeclTemplate.h
===
--- clang/include/clang/AST/DeclTemplate.h
+++ clang/include/clang/AST/DeclTemplate.h
@@ -1148,23 +1148,40 @@
 /// parameters and is not part of the Decl hierarchy. Just a facility.
 class TemplateParmPosition {
 protected:
-  // FIXME: These probably don't need to be ints. int:5 for depth, int:8 for
-  // position? Maybe?
-  unsigned Depth;
-  unsigned Position;
+  enum { DepthWidth = 20, PositionWidth = 12 };
+  unsigned Depth : DepthWidth;
+  unsigned Position : PositionWidth;
 
-  TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {}
+  static constexpr unsigned MaxDepth = (1U << DepthWidth) - 1;
+  static constexpr unsigned MaxPosition = (1U << PositionWidth) - 1;
+
+  TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {
+// The input may fill maximum values to show that it is invalid.
+// Add one here to convert it to zero.
+assert((D + 1) <= MaxDepth &&
+   "The depth of template parmeter position is more than 2^20!");
+assert((P + 1) <= MaxPosition &&
+   "The position of template parmeter position is more than 2^12!");
+  }
 
 public:
   TemplateParmPosition() = delete;
 
   /// Get the nesting depth of the template parameter.
   unsigned getDepth() const { return Depth; }
-  void setDepth(unsigned D) { Depth = D; }
+  void setDepth(unsigned D) {
+assert((D + 1) <= MaxDepth &&
+   "The depth of template parmeter position is more than 2^20!");
+Depth = D;
+  }
 
   /// Get the position of the template parameter within its parameter list.
   unsigned getPosition() const { return Position; }
-  void setPosition(unsigned P) { Position = P; }
+  void setPosition(unsigned P) {
+assert((P + 1) <= MaxPosition &&
+   "The position of template parmeter position is more than 2^12!");
+Position = P;
+  }
 
   /// Get the index of the template parameter within its parameter list.
   unsigned getIndex() const { return Position; }


Index: clang/include/clang/AST/DeclTemplate.h
===
--- clang/include/clang/AST/DeclTemplate.h
+++ clang/include/clang/AST/DeclTemplate.h
@@ -1148,23 +1148,40 @@
 /// parameters and is not part of the Decl hierarchy. Just a facility.
 class TemplateParmPosition {
 protected:
-  // FIXME: These probably don't need to be ints. int:5 for depth, int:8 for
-  // position? Maybe?
-  unsigned Depth;
-  unsigned Position;
+  enum { DepthWidth = 20, PositionWidth = 12 };
+  unsigned Depth : DepthWidth;
+  unsigned Position : PositionWidth;
 
-  TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {}
+  static constexpr unsigned MaxDepth = (1U << DepthWidth) - 1;
+  static constexpr unsigned MaxPosition = (1U << PositionWidth) - 1;
+
+  TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {
+// The input may fill maximum values to show that it is invalid.
+// Add one here to convert it to zero.
+assert((D + 1) <= MaxDepth &&
+   "The depth of template parmeter position is more than 2^20!");
+assert((P + 1) <= MaxPosition &&
+   "The position of template parmeter position is more than 2^12!");
+  }
 
 public:
   TemplateParmPosition() = delete;
 
   /// Get the nesting depth of the template parameter.
   unsigned getDepth() const { return Depth; }
-  void setDepth(unsigned D) { Depth = D; }
+  void setDepth(unsigned D) {
+assert((D + 1) <= MaxDepth &&
+   "The depth of template parmeter position is more than 2^20!");
+Depth = D;
+  }
 
   /// Get the position of the template parameter within its parameter list.
   unsigned getPosition() const { return Position; }
-  void setPosition(unsigned P) { Position = P; }
+  void setPosition(unsigned P) {
+assert((P + 1) <= MaxPosition &&
+   "The position of template parmeter position is more than 2^12!");
+Position = P;
+  }
 
   /// Get the index of the template parameter within its parameter list.
   unsigned getIndex() const { return Position; }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/AST/DeclTemplate.h:1151-1152
 protected:
-  // FIXME: These probably don't need to be ints. int:5 for depth, int:8 for
-  // position? Maybe?
-  unsigned Depth;
-  unsigned Position;
+#define DEPTH_BITWIDTH 20
+#define POSITION_BITWIDTH 12
+  unsigned Depth : DEPTH_BITWIDTH;

Just as constant as a macro, but without all the macro issues.



Comment at: clang/include/clang/AST/DeclTemplate.h:1156-1157
 
-  TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {}
+  static constexpr unsigned MaxDepth = (1l << DEPTH_BITWIDTH) - 1;
+  static constexpr unsigned MaxPosition = (1l << POSITION_BITWIDTH) - 1;
+

ChuanqiXu wrote:
> I don't find standard way to generate the maximum value for bit fields.. So I 
> choose to calculate it by hand. 
Since we're adding 1 everywhere we look at these values, would it make more 
sense to drop the -1 here and the +1 elsewhere?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123298/new/

https://reviews.llvm.org/D123298

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

> On the other hand, why not use 16 for both?

I am consistent with @aaron.ballman here.




Comment at: clang/include/clang/AST/DeclTemplate.h:1156-1157
 
-  TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {}
+  static constexpr unsigned MaxDepth = (1l << DEPTH_BITWIDTH) - 1;
+  static constexpr unsigned MaxPosition = (1l << POSITION_BITWIDTH) - 1;
+

I don't find standard way to generate the maximum value for bit fields.. So I 
choose to calculate it by hand. 



Comment at: clang/include/clang/AST/DeclTemplate.h:1163-1164
+  TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {
+// The input may fill maximum values to show that it is invalid.
+// Add one here to convert it to zero.
+assert((D + 1) <= MaxDepth &&

I find it is possible if ast-dump is going to dump from JSON files.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123298/new/

https://reviews.llvm.org/D123298

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 421419.
ChuanqiXu added a comment.

Address comments:

- Add assertions to show the input wouldn't hit the boundaries.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123298/new/

https://reviews.llvm.org/D123298

Files:
  clang/include/clang/AST/DeclTemplate.h


Index: clang/include/clang/AST/DeclTemplate.h
===
--- clang/include/clang/AST/DeclTemplate.h
+++ clang/include/clang/AST/DeclTemplate.h
@@ -1148,23 +1148,44 @@
 /// parameters and is not part of the Decl hierarchy. Just a facility.
 class TemplateParmPosition {
 protected:
-  // FIXME: These probably don't need to be ints. int:5 for depth, int:8 for
-  // position? Maybe?
-  unsigned Depth;
-  unsigned Position;
+#define DEPTH_BITWIDTH 20
+#define POSITION_BITWIDTH 12
+  unsigned Depth : DEPTH_BITWIDTH;
+  unsigned Position : POSITION_BITWIDTH;
 
-  TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {}
+  static constexpr unsigned MaxDepth = (1l << DEPTH_BITWIDTH) - 1;
+  static constexpr unsigned MaxPosition = (1l << POSITION_BITWIDTH) - 1;
+
+#undef DEPTH_BITWIDTH
+#undef POSITION_BITWIDTH
+
+  TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {
+// The input may fill maximum values to show that it is invalid.
+// Add one here to convert it to zero.
+assert((D + 1) <= MaxDepth &&
+   "The depth of template parmeter position is more than 2^20!");
+assert((P + 1) <= MaxPosition &&
+   "The position of template parmeter position is more than 2^12!");
+  }
 
 public:
   TemplateParmPosition() = delete;
 
   /// Get the nesting depth of the template parameter.
   unsigned getDepth() const { return Depth; }
-  void setDepth(unsigned D) { Depth = D; }
+  void setDepth(unsigned D) {
+assert((D + 1) <= MaxDepth &&
+   "The depth of template parmeter position is more than 2^20!");
+Depth = D;
+  }
 
   /// Get the position of the template parameter within its parameter list.
   unsigned getPosition() const { return Position; }
-  void setPosition(unsigned P) { Position = P; }
+  void setPosition(unsigned P) {
+assert((P + 1) <= MaxPosition &&
+   "The position of template parmeter position is more than 2^12!");
+Position = P;
+  }
 
   /// Get the index of the template parameter within its parameter list.
   unsigned getIndex() const { return Position; }


Index: clang/include/clang/AST/DeclTemplate.h
===
--- clang/include/clang/AST/DeclTemplate.h
+++ clang/include/clang/AST/DeclTemplate.h
@@ -1148,23 +1148,44 @@
 /// parameters and is not part of the Decl hierarchy. Just a facility.
 class TemplateParmPosition {
 protected:
-  // FIXME: These probably don't need to be ints. int:5 for depth, int:8 for
-  // position? Maybe?
-  unsigned Depth;
-  unsigned Position;
+#define DEPTH_BITWIDTH 20
+#define POSITION_BITWIDTH 12
+  unsigned Depth : DEPTH_BITWIDTH;
+  unsigned Position : POSITION_BITWIDTH;
 
-  TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {}
+  static constexpr unsigned MaxDepth = (1l << DEPTH_BITWIDTH) - 1;
+  static constexpr unsigned MaxPosition = (1l << POSITION_BITWIDTH) - 1;
+
+#undef DEPTH_BITWIDTH
+#undef POSITION_BITWIDTH
+
+  TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {
+// The input may fill maximum values to show that it is invalid.
+// Add one here to convert it to zero.
+assert((D + 1) <= MaxDepth &&
+   "The depth of template parmeter position is more than 2^20!");
+assert((P + 1) <= MaxPosition &&
+   "The position of template parmeter position is more than 2^12!");
+  }
 
 public:
   TemplateParmPosition() = delete;
 
   /// Get the nesting depth of the template parameter.
   unsigned getDepth() const { return Depth; }
-  void setDepth(unsigned D) { Depth = D; }
+  void setDepth(unsigned D) {
+assert((D + 1) <= MaxDepth &&
+   "The depth of template parmeter position is more than 2^20!");
+Depth = D;
+  }
 
   /// Get the position of the template parameter within its parameter list.
   unsigned getPosition() const { return Position; }
-  void setPosition(unsigned P) { Position = P; }
+  void setPosition(unsigned P) {
+assert((P + 1) <= MaxPosition &&
+   "The position of template parmeter position is more than 2^12!");
+Position = P;
+  }
 
   /// Get the index of the template parameter within its parameter list.
   unsigned getIndex() const { return Position; }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I note that 'recursive' depth is limited to 1024: 
https://godbolt.org/z/h9bEfsToT

Though I think there are other ways to get 'deeper' than that.  So perhaps in 
any case we're just arranging deck chairs on a -post-error titanic :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123298/new/

https://reviews.llvm.org/D123298

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

> I think people instantiate to deeper template depths than they typically have 
> for template parameters, so having a deeper depth made sense to me.

Hmm... the cases I can think of make me think this isn't necessarily true... It 
is very common to template-recurse on an integer-sequence of some sort, which 
would result in depth and position being roughly the same.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123298/new/

https://reviews.llvm.org/D123298

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

[Aaron Ballman via cfe-commits]

On Thu, Apr 7, 2022 at 8:35 AM Corentin via cfe-commits
 wrote:
>
>
>
> On Thu, Apr 7, 2022 at 2:11 PM Aaron Ballman via Phabricator 
>  wrote:
>>
>> aaron.ballman added a comment.
>>
>> In D123298#3435796 , @cor3ntin 
>> wrote:
>>
>> > In D123298#3435770 , 
>> > @aaron.ballman wrote:
>> >
>> >> Changes LGTM, I also don't think we should hit these limits. Perhaps we 
>> >> should add some assertions to the ctor and the setter functions just to 
>> >> be sure though?
>> >
>> > If we are going to do anything, it ought to be a diagnostic?
>>
>> Doing a diagnostic would mean finding all the places where we form a 
>> `TemplateParmPosition` and ensure we have enough source location information 
>> to issue the diagnostic. Given that we don't expect users to ever hit it, 
>> having the assertion gives a wee bit of coverage (godbolt exposes an 
>> assertions build, for example) but without as much implementation burden. 
>> That said, if it's easy enough to give diagnostics, that's a more 
>> user-friendly approach if we think anyone would hit that limit. (I suspect 
>> template instantiation depth would be hit before bumping up against these 
>> limits, though.)
>
>
> Fair enough - I suspect godbolt would die in that scenario though.
> Note that i was not asking for a diagnostic, just saying that the assertion 
> may not protect anyone

Agreed, it's a pretty paltry protection. :-) My thinking is: we likely
have torture test suite cases that may tell us if we've guessed wrong
(either in tree, or in some downstream).

>> > I can't imagine a scenario in which someone would hit these limits and 
>> > have assertions enabled. But i agree with you that the limit themselves 
>> > should not be hit.
>> > On the other hand, why not use 16 for both?
>>
>> I think people instantiate to deeper template depths than they typically 
>> have for template parameters, so having a deeper depth made sense to me.
>
> Sure, but there is a huge imbalance there. 1048576 vs 4096 - I think it's 
> still better than msvc and it's conforming - the standard sets the minimum at 
> 1024 for both afaik

That's a fair point. I don't have a strong opinion on what bit widths
we use so long as they are "sensible" (for some definition of the
term).
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

[Corentin via cfe-commits]

On Thu, Apr 7, 2022 at 2:11 PM Aaron Ballman via Phabricator <
revi...@reviews.llvm.org> wrote:

> aaron.ballman added a comment.
>
> In D123298#3435796 , @cor3ntin
> wrote:
>
> > In D123298#3435770 ,
> @aaron.ballman wrote:
> >
> >> Changes LGTM, I also don't think we should hit these limits. Perhaps we
> should add some assertions to the ctor and the setter functions just to be
> sure though?
> >
> > If we are going to do anything, it ought to be a diagnostic?
>
> Doing a diagnostic would mean finding all the places where we form a
> `TemplateParmPosition` and ensure we have enough source location
> information to issue the diagnostic. Given that we don't expect users to
> ever hit it, having the assertion gives a wee bit of coverage (godbolt
> exposes an assertions build, for example) but without as much
> implementation burden. That said, if it's easy enough to give diagnostics,
> that's a more user-friendly approach if we think anyone would hit that
> limit. (I suspect template instantiation depth would be hit before bumping
> up against these limits, though.)
>

Fair enough - I suspect godbolt would die in that scenario though.
Note that i was not asking for a diagnostic, just saying that the assertion
may not protect anyone


>
> > I can't imagine a scenario in which someone would hit these limits and
> have assertions enabled. But i agree with you that the limit themselves
> should not be hit.
> > On the other hand, why not use 16 for both?
>
> I think people instantiate to deeper template depths than they typically
> have for template parameters, so having a deeper depth made sense to me.
>

Sure, but there is a huge imbalance there. 1048576 vs 4096 - I think it's
still better than msvc and it's conforming - the standard sets the minimum
at 1024 for both afaik


>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D123298/new/
>
> https://reviews.llvm.org/D123298
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D123298#3435796 , @cor3ntin wrote:

> In D123298#3435770 , @aaron.ballman 
> wrote:
>
>> Changes LGTM, I also don't think we should hit these limits. Perhaps we 
>> should add some assertions to the ctor and the setter functions just to be 
>> sure though?
>
> If we are going to do anything, it ought to be a diagnostic?

Doing a diagnostic would mean finding all the places where we form a 
`TemplateParmPosition` and ensure we have enough source location information to 
issue the diagnostic. Given that we don't expect users to ever hit it, having 
the assertion gives a wee bit of coverage (godbolt exposes an assertions build, 
for example) but without as much implementation burden. That said, if it's easy 
enough to give diagnostics, that's a more user-friendly approach if we think 
anyone would hit that limit. (I suspect template instantiation depth would be 
hit before bumping up against these limits, though.)

> I can't imagine a scenario in which someone would hit these limits and have 
> assertions enabled. But i agree with you that the limit themselves should not 
> be hit. 
> On the other hand, why not use 16 for both?

I think people instantiate to deeper template depths than they typically have 
for template parameters, so having a deeper depth made sense to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123298/new/

https://reviews.llvm.org/D123298

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

In D123298#3435770 , @aaron.ballman 
wrote:

> Changes LGTM, I also don't think we should hit these limits. Perhaps we 
> should add some assertions to the ctor and the setter functions just to be 
> sure though?

If we are going to do anything, it ought to be a diagnostic? 
I can't imagine a scenario in which someone would hit these limits and have 
assertions enabled. But i agree with you that the limit themselves should not 
be hit. 
On the other hand, why not use 16 for both?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123298/new/

https://reviews.llvm.org/D123298

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

Changes LGTM, I also don't think we should hit these limits. Perhaps we should 
add some assertions to the ctor and the setter functions just to be sure though?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123298/new/

https://reviews.llvm.org/D123298

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu created this revision.
ChuanqiXu added reviewers: erichkeane, aaron.ballman, cor3ntin.
ChuanqiXu added a project: clang.
Herald added a subscriber: kristof.beyls.
Herald added a project: All.
ChuanqiXu requested review of this revision.
Herald added a subscriber: cfe-commits.

I found this when reading the codes. I think it makes sense to reduce the space 
for TemplateParmPosition. It is hard to image the depth of template parameter 
is larger than 2^20 and the index is larger than 2^12. So I think the patch 
might be reasonable.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D123298

Files:
  clang/include/clang/AST/DeclTemplate.h


Index: clang/include/clang/AST/DeclTemplate.h
===
--- clang/include/clang/AST/DeclTemplate.h
+++ clang/include/clang/AST/DeclTemplate.h
@@ -1148,10 +1148,8 @@
 /// parameters and is not part of the Decl hierarchy. Just a facility.
 class TemplateParmPosition {
 protected:
-  // FIXME: These probably don't need to be ints. int:5 for depth, int:8 for
-  // position? Maybe?
-  unsigned Depth;
-  unsigned Position;
+  unsigned Depth : 20;
+  unsigned Position : 12;
 
   TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {}
 


Index: clang/include/clang/AST/DeclTemplate.h
===
--- clang/include/clang/AST/DeclTemplate.h
+++ clang/include/clang/AST/DeclTemplate.h
@@ -1148,10 +1148,8 @@
 /// parameters and is not part of the Decl hierarchy. Just a facility.
 class TemplateParmPosition {
 protected:
-  // FIXME: These probably don't need to be ints. int:5 for depth, int:8 for
-  // position? Maybe?
-  unsigned Depth;
-  unsigned Position;
+  unsigned Depth : 20;
+  unsigned Position : 12;
 
   TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {}
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits