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:
> aeubanks wrote:
> > 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
> Note that 1% is the increase in the files in `clang/lib/CodeGen`. The files
> in other directories (e.g., `clang/lib/Sema`) aren't affected by the change
> as they don't use `Address`.
>
> I see an increase of 0.33% in max RSS when I run the tests in
> `clang/test/CodeGen*`, but I'm not sure how reliable that is.
>
> How do you run the potential patch through `llvm-compile-time-tracker`? The
> patch just changes the type to `AddressImpl<void, false> A;` on this line:
>
> https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/Address.h#L92
>
>
https://llvm-compile-time-tracker.com/compare.php?from=998ad085e865f2e5acc589d6bee0e3379042da2e&to=5de4a1989c474f37ac03f20ccb0aef50f6e3b854&stat=max-rss
seems fine to me
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142584/new/
https://reviews.llvm.org/D142584
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
