[PATCH] D142584: [CodeGen] Add a boolean flag to `Address::getPointer` and `Lvalue::getPointer` that indicates whether the pointer is known not to be null

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.


================
Comment at: clang/lib/CodeGen/Address.h:67
       return;
-    // Currently the max supported alignment is much less than 1 << 63 and is
+    // Currently the max supported alignment is much less than 1 << 32 and is
     // guaranteed to be a power of 2, so we can store the log of the alignment
----------------
ahatanak wrote:
> ahatanak wrote:
> > efriedma wrote:
> > > ahatanak wrote:
> > > > efriedma wrote:
> > > > > ahatanak wrote:
> > > > > > efriedma wrote:
> > > > > > > This comment isn't right.  The max alignment is, as far as I can tell, 1<<32 exactly.  (But there's something weird going on with very large values... somehow `int a[1LL<<32] __attribute((aligned(1ULL<<32))) = {};` ignores the alignment.)
> > > > > > The following function generated by tablegen (and a few others directly or indirectly calling the function) returns a 32-bit int, but it should be returning a 64-bit int.
> > > > > > 
> > > > > > https://github.com/llvm/llvm-project/blob/main/clang/utils/TableGen/ClangAttrEmitter.cpp#L532
> > > > > Filed https://github.com/llvm/llvm-project/issues/60752 so we don't lose track of this.
> > > > I just realized we can't reduce the number of bits used for alignment here as we need 6 bits for alignment of `1 << 32`.
> > > > 
> > > > Should we allocate additional memory when `AlignLog` is either 31 or 32? If the 5-bit alignment is equal to `0b11111`, it would mean that there is an out-of-line storage large enough to hold the alignment and any other extra information that is needed. I think https://reviews.llvm.org/D117262#3267899 proposes a similar idea.
> > > How much does `sizeof(Address)` actually matter, anyway?  If it's going to get that nasty to implement the packing, I'm not sure it's worth the effort to optimize.
> > I'm not sure, but apparently https://reviews.llvm.org/D117262 was needed to reduce the memory usage.
> > 
> > I don't see a significant increase in stack usage (a little over 1%) in the files in `clang/lib/CodeGen` when I build clang with `-fstack-usage`.
> @aeubanks do we have to use the specialization `AddressImpl<T, true>`?
> 
> https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/Address.h#L31
looking back at the internal bug motivating the patch, it was because some we had to lower some thresholds in a stress test, so that in it of itself isn't super important. but `Address` is used a lot and it's nice to keep resource usage down (1% isn't trivial). does this noticeably affect max RSS? could run a potential patch through llvm-compile-time-tracker (I can help with that) to see the impact


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142584