[PATCH] D64790: [LLVM][NFC] Adding an Alignment type to LLVM

JF Bastien via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jul 30 09:09:48 PDT 2019


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

Overall this looks good, thanks!



================
Comment at: llvm/include/llvm/Support/Alignment.h:40
+// It is suitable for use as static global constants.
+struct Align final {
+private:
----------------
gchatelet wrote:
> jfb wrote:
> > `final` is kinda weird here. Not that I'd inherit from this... but why have `final`?
> It was a way to document that this type is not to be derived from. It does not have a virtual destructor.
> Now I don't have a strong opinion and I'm happy to remove it if you think it's too weird.
> 
It's just not something we really do anywhere.


================
Comment at: llvm/include/llvm/Support/Alignment.h:146
+// Returns unsigned(-1) if current alignment is undefined.
+inline unsigned Log2(MaybeAlign A) { return A ? Log2(A.getValue()) : -1; }
+
----------------
gchatelet wrote:
> jfb wrote:
> > Are the places where `Log2` with undefined alignment is called today? Can we report fatal error instead (or assert)?
> Since I wanted to first introduce the type and then fix invalid code I never had a chance to run LLVM with all the `assert`s.
> By looking at the code it is definitely possible but I can't tell for sure if it happens. It's one of the motivation for introducing the type in the first place.
> 
> For instance it is used in [TargetCallingConv](https://github.com/llvm/llvm-project/blob/074db9b8e97aefbd7cd95f856dd1b06d3ec19fd1/llvm/include/llvm/CodeGen/TargetCallingConv.h#L125) where `A` may well be `0`. In that case `Log2_32` [will return -1](https://github.com/llvm/llvm-project/blob/c75cdd056f6993bfed0962845e2f79f062a4bc11/llvm/include/llvm/Support/MathExtras.h#L537), hence the `+ 1` to encode invalid as zero.
> 
> Now that it's clear we want to fix the code as we introduce the type, we can treat it as an error as you suggest.
Thanks, I think this is probably the right approach since it forces the changes as adoption happens.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64790





More information about the llvm-commits mailing list