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

Wed Jul 17 05:44:36 PDT 2019

gchatelet marked 8 inline comments as done.
gchatelet added a comment.

A few comments for now, I'll reply to the other ones soonish.

================
Comment at: llvm/include/llvm/Support/Alignment.h:69
+// alignment.
+// Its minimum value is 1, its maximum value is 2^31, 2^32 is excluded on
+// purpose to prevent mixing it up with -1.
----------------
jakehehrlich wrote:
> 2^32 isn't a value that uint32_t can represent so you might change this. 2^31 is the largest valid power of two, 2^32 - 1 is the largest value that can be represented. I don't think you need to bring negative values in to this since its all using uint32_t. Also my math on this is rusty but I think int32_t(uint32_t(1) << uint32_t(31)) == -1.
Thx for noticing, this comment is nonsense indeed.
-1 is all ones and can't be a power of two so there's no need bothering.

================
Comment at: llvm/include/llvm/Support/Alignment.h:94
+  // A safe halving method that makes sure semantic is preserved.
+  Align half() const {
+    assert(Value > 1 && "Can't halve byte alignment");
----------------
jakehehrlich wrote:
> Is halving ever really needed?
Yes, all the methods here correspond to real code encountered while refactoring `llvm-project`.
 - https://github.com/llvm/llvm-project/blob/9eb95903da4575b3b95d1a743d48e51bb5026ccd/llvm/lib/CodeGen/CodeGenPrepare.cpp#L6751
 - https://github.com/llvm/llvm-project/blob/9eb95903da4575b3b95d1a743d48e51bb5026ccd/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L16529
 - https://github.com/llvm/llvm-project/blob/9eb95903da4575b3b95d1a743d48e51bb5026ccd/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp#L5823
 - https://github.com/llvm/llvm-project/blob/9eb95903da4575b3b95d1a743d48e51bb5026ccd/llvm/lib/Target/ARM/ARMFrameLowering.cpp#L2054
 - https://github.com/llvm/llvm-project/blob/9eb95903da4575b3b95d1a743d48e51bb5026ccd/llvm/lib/Target/ARM/ARMISelLowering.cpp#L11753
 - https://github.com/llvm/llvm-project/blob/9eb95903da4575b3b95d1a743d48e51bb5026ccd/llvm/lib/Target/ARM/ARMISelLowering.cpp#L12494

Would you prefer `operator/` and `operator/=`?

Please keep in mind that I didn't try to refactor the original code, the first step was introduce the type without touching too much code. The patch corresponding to the whole change is already 26K lines.

================
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:
> 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?

================
Comment at: llvm/include/llvm/Support/Alignment.h:107
+  // Needed to interact with C for instance.
+  uint64_t value() const { return Value; }
+};
----------------
jakehehrlich wrote:
> If size is a concern yet we only want to represent powers of two I think encoding the power two only would let us represent 64-bit alignments using only a 1 byte value at the cost of a slight bit of increased computation on each operation. These operations tend to not be very performance critical however.
Yes that's a possibility I wanted to explore as well, but I'm not sure about the tradeoffs at this point.
IMHO the code would be much more straightforward compared to a templated version.

**I'm interested in comments from other developers here**.

Storing alignment as a byte would allow alignment up to 2^254
```
0:0
1:1
2:2
3:4
4:8
...
255: 2^254
```

================
Comment at: llvm/include/llvm/Support/Alignment.h:115
+// It is suitable for use as static global constants.
+struct MaybeAlign final {
+private:
----------------
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.

================
Comment at: llvm/include/llvm/Support/Alignment.h:157-159
+  void mustBeSetAtThisPoint() const {
+    assert(Value && "Alignment must be set at this point");
+  }
----------------
jakehehrlich wrote:
> I don't think you should be providing this. it's easy enough to use "isSet" and their own assert. If I was reading this I wouldn't expect this to be an assert under the hood and nothing else.
SGTM will remove

================
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:
> 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.

================
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:
> 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.

================
Comment at: llvm/include/llvm/Support/Alignment.h:210
+// once the call sites are updated.
+ALIGN_DEPRECATED(
+    bool isAligned(const MaybeAlign &Lhs, uint64_t SizeInBytes),
----------------
jakehehrlich wrote:
> I see a lot of "bogus" or "dangerous" or "deprecated" things being added and these all huge big red flags to me. Can you better justify the need for them?
Same as previous comment, this is actual code that seems to be wrong but not to be fixed in this patch.
e.g. of `isAlignedBogus` https://github.com/llvm/llvm-project/blob/8b7041a5c6f0a373d4886ca807d89790ad6dedab/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp#L8701

================
Comment at: llvm/include/llvm/Support/Alignment.h:251
+
+inline uint64_t alignTo(uint64_t Size, const Align &A) {
+  return (Size + A.value() - 1) / A.value() * A.value();
----------------
jakehehrlich wrote:
> I'd probably not use const Align & and instead just use Align. This matches how alignment is used over the code base anyhow things like ArrayRef and StringRef are used.
SGTM

================
Comment at: llvm/include/llvm/Support/Alignment.h:252
+inline uint64_t alignTo(uint64_t Size, const Align &A) {
+  return (Size + A.value() - 1) / A.value() * A.value();
+}
----------------
jakehehrlich wrote:
> You can use `(Size + A.value() - 1) & -A.value()` since you explicitly only represent powers of two. Not critical by any means but a possible implementation that saves on division operations which can be almost as slow as an allocation.
> 
> You can think of what you're doing as being like a right shift followed by a left shift which zeros out the extra bits produced in the bump. `-A.value()` is a mask where all those bits being zeroed out are just zero. I've found that both myself and many other people get confused by this trick so a comment is worth while if you use it.
I'm happy to optimize this afterwards if you don't mind. The review will be easier if I just move code around without changing it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64790