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

Fri Jul 19 07:44:42 PDT 2019

gchatelet marked 32 inline comments as done.
gchatelet added inline comments.

================
Comment at: llvm/include/llvm/Support/Alignment.h:101-103
+  bool needsPadding(uint64_t Size) const {
+    return (Size & (uint64_t(Value) - 1)) != 0;
+  }
----------------
jakehehrlich wrote:
> gchatelet wrote:
> > jakehehrlich wrote:
> > > Seems like you'd just want to provide helper functions like "alignTo" that do this for you. There's a whole family of them but personally I've primarily used "alignTo" and one other really odd highly specific one for ELF to deal with segment alignment.
> > I've encountered this pattern a number of times and considered it was worth factoring.
> >  - https://github.com/llvm/llvm-project/blob/9eb95903da4575b3b95d1a743d48e51bb5026ccd/llvm/lib/IR/DataLayout.cpp#L58
> >  - https://github.com/llvm/llvm-project/blob/9eb95903da4575b3b95d1a743d48e51bb5026ccd/llvm/lib/IR/DataLayout.cpp#L75
> >  - https://github.com/llvm/llvm-project/blob/9eb95903da4575b3b95d1a743d48e51bb5026ccd/llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp#L506
> >  - https://github.com/llvm/llvm-project/blob/9eb95903da4575b3b95d1a743d48e51bb5026ccd/llvm/lib/Transforms/Coroutines/CoroFrame.cpp#L329
> > 
> > I agree it doesn't have to be a struct member and can be changed to a free function. Any suggestion for a better name?
> Those are all easily refactored. I'm not sure how "IsPadded" is being propogated though but if that's needed you can check to see if the value changes. In general needing to check for padding like most of those examples are is anti pattern. The error checking case can just be `X != alignTo(X, Align)`
SGTM, I'll remove it.

================
Comment at: llvm/include/llvm/Support/Alignment.h:115
+// It is suitable for use as static global constants.
+struct MaybeAlign final {
+private:
----------------
jakehehrlich wrote:
> gchatelet wrote:
> > jakehehrlich wrote:
> > > What's the use case for this type? I don't think any ELF tools have a use case for this at least. ELF uses 0 to mean the same thing as 1 so we'd just want something that converted to 1 asap.
> > > 
> > > I think its interface should copy Optional if this has a use case. e.g. using Optional<Align> should function close to the same, perhaps with some operations from Align that act in sensible ways.
> > Yes semantically this is exactly Optional<Align>, the use of this type is pervasive in LLVM where the alignment requirements are first unknown and set down the road when needed.
> > Problem is `llvm::Optional` needs an additional `bool` to know if the value is set or not so `MaybeAlign` is more compact than `Optional<Align>` (https://godbolt.org/z/8_uA_D)  it's also more customizable.
> Right I understand the optimization I'm saying you should match the interface modulo a few operations specific to alignments.
Acknowledged. Will do.

================
Comment at: llvm/include/llvm/Support/Alignment.h:179-181
+  ALIGN_DEPRECATED(
+      bool needsPadding(uint64_t Size) const,
+      "needsPadding involving MaybeAlign mixes semantics, use Align instead");
----------------
jakehehrlich wrote:
> gchatelet wrote:
> > jakehehrlich wrote:
> > > Why add a deprecated method to new code? Nothing is using this so there's no need for backward compatibility. Also the two methods above look equally dangerous but boil down to Optional's 'get' or dereference.
> > ALIGN_DEPRECATED is defined only when ALIGN_HARDENED_ALIGNMENT is set.
> > As of this patch there will be no deprecation message, it's useful as a second step to find fishy code.
> > Fixing the code while introducing the type is not feasible considering the patch is already humongous.
> You can just remove all the deprecated methods and then gradually refactor other code. Nothing calls this yet so there's no reason to add deprecated functionality.
I  understand your reluctance to see these deprecated methods in new code but I've went through the exercise of the whole refactoring. It took me about 4 weeks of full time job. This change is not light, it is massive (patch is 26 000 lines). My decision to go this route was to make the review as straightforward as possible so it has a chance to happen. I also wanted to leave breadcrumbs along the way so fishy code could be fixed as a second step.

Since you seem to feel strongly about this I will remove the deprecated methods but I'm not so sure this refactoring will happen thoroughly considering the required investment.

================
Comment at: llvm/include/llvm/Support/Alignment.h:192-194
+inline bool MaybeAlign::needsPadding(uint64_t Size) const {
+  return Value ? Align(Value).needsPadding(Size) : false;
+}
----------------
jakehehrlich wrote:
> gchatelet wrote:
> > jakehehrlich wrote:
> > > Why does choosing false make sense here? It isn't clear to me what should be returned here one way or the other. If padding is needed then one would assume you'd also need a method to perform the padding required. If this was an empty alignment how could you do that?
> > I concur this does not make sense but this is what the code is doing right now. Since this patch is about introducing a type and not fix the code, I've to add this function for now.
> > This is why these functions will be deprecated afterwards.
> You should introduce a new proper type, and fix code as its migrated over, not introduce more broken code and gradually unbreak it.
I don't think it introduces "more broken code" since it factors the broken code in a single place but I understand your position and I will try to follow that path.
I'm worried that the incremental fixing may take forever and that I may be dragged away before finishing it.

================
Comment at: llvm/include/llvm/Support/Alignment.h:262
+// Returns `Size` if current alignment is undefined.
+inline uint64_t alignTo(uint64_t Size, const MaybeAlign &A) {
+  return A ? alignTo(Size, A.get()) : Size;
----------------
jakehehrlich wrote:
> I think this being the default makes sense but it would be worth showing examples of where this is used. This for instance matches what ELF should do with zero alignment but thats really just the same as saying that what MaybeAlignment calls undefined is actully an alignment of 1.
IMHO ELF shouldn't use the `MaybeAlign` type since it represent something that is not yet defined.
AFAIU ELF always deals with known alignment so it should use `Align` that has well defined semantics.
I envision an `Align assumeAligned(uint64_t)` function that will make `0` be a `Align(1)`

================
Comment at: llvm/include/llvm/Support/Alignment.h:321
+// e.g. Align(16).encode() == 5
+inline unsigned encode(const Align &A) { return Log2_32(A.value()) + 1; }
+
----------------
jakehehrlich wrote:
> 1) Does any existing code use this
> 2) Why add the '1'? Id expect this to be the inverse of `1 << x` but instead its the inverse of `1 << (x - 1)` or as you do below `(1 << x) >> 1`
1

Unfortunately yes, here are a few of them
 - https://github.com/llvm/llvm-project/blob/fecf43eba3630aeb55c56e5d308f99a7bd05bfbe/llvm/lib/CodeGen/MachineOperand.cpp#L1039
 - https://github.com/llvm/llvm-project/blob/fecf43eba3630aeb55c56e5d308f99a7bd05bfbe/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp#L2911
 - https://github.com/llvm/llvm-project/blob/fecf43eba3630aeb55c56e5d308f99a7bd05bfbe/llvm/lib/CodeGen/MachineOperand.cpp#L1005
 - https://github.com/llvm/llvm-project/blob/fecf43eba3630aeb55c56e5d308f99a7bd05bfbe/llvm/lib/IR/Instructions.cpp#L1247

2

I guess it's because they also want to encode the `undefined` as 0 as it is common in LLVM.

================
Comment at: llvm/include/llvm/Support/Alignment.h:339
+// Comparisons between Align and scalars. Rhs must be positive.
+inline bool operator==(const Align &Lhs, uint64_t Rhs) {
+  ALIGN_CHECK_ISPOSITIVE(Rhs);
----------------
jakehehrlich wrote:
> Would be nice to have a version for ELF specifically that treated '0' and '1' as equal. 
I'm not sure about this one, I would encourage using the `bool operator==(Align Lhs, Align Rhs)` version with the `assumeAligned()` function I was talking earlier - and not compare directly with 0.

================
Comment at: llvm/include/llvm/Support/Alignment.h:441
+// Comparisons operators between MaybeAlign and Align.
+inline bool operator==(const MaybeAlign &Lhs, const Align &Rhs) {
+  ALIGN_CHECK_ISSET(Lhs);
----------------
jakehehrlich wrote:
> How does `Optional<T>` handle equality in the `None` case? I think we should match those semantics....maybe depends on what they are.
Here are the definitions: https://llvm.org/doxygen/Optional_8h_source.html
I've tightened the defintions for MaybeAlign compared to Optional: the code will check fail when comparing the None case to a real value.

================
Comment at: llvm/include/llvm/Support/Alignment.h:501
+// The maximum of two valid alignments is a valid alignment.
+inline Align max(const Align &A, const Align &B) { return A > B ? A : B; }
+
----------------
jakehehrlich wrote:
> Won't `std::max` do this automatically since you implemented the comparison operators?
Yes I removed them.

================
Comment at: llvm/unittests/Support/AlignmentTest.cpp:30-31
+std::vector<uint64_t> getAlignmentTooBig() {
+  return {std::numeric_limits<uint32_t>::max() /*2^32*/,
+          std::numeric_limits<uint64_t>::max() /*2^64*/};
+}
----------------
jakehehrlich wrote:
> These values are wrong and not powers of two.
Got rid of them.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64790