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

Tue Jul 16 14:52:18 PDT 2019

jakehehrlich added inline comments.

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

================
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");
----------------
Is halving ever really needed?

================
Comment at: llvm/include/llvm/Support/Alignment.h:101-103
+  bool needsPadding(uint64_t Size) const {
+    return (Size & (uint64_t(Value) - 1)) != 0;
+  }
----------------
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.

================
Comment at: llvm/include/llvm/Support/Alignment.h:107
+  // Needed to interact with C for instance.
+  uint64_t value() const { return Value; }
+};
----------------
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.

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

================
Comment at: llvm/include/llvm/Support/Alignment.h:157-159
+  void mustBeSetAtThisPoint() const {
+    assert(Value && "Alignment must be set at this point");
+  }
----------------
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.

================
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");
----------------
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.

================
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;
+}
----------------
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?

================
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),
----------------
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?

================
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();
----------------
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.

================
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();
+}
----------------
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.

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

================
Comment at: llvm/include/llvm/Support/Alignment.h:271
+
+inline unsigned Log2_32(const Align &A) { return Log2_32(A.value()); }
+
----------------
Is there a reason to 1) copy the naming convention here or 2) to not just call this "Log2"? Also this operation will just be "return value" if you use the 1-byte representation proposed above.

A merit of using the bit shift representation is that the cost of converting to the origional is low (`1 << value`) but using the current representation `Log2_32` is quite complicated under the hood and requires a large handful of bitwise operations.

================
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; }
+
----------------
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`

================
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);
----------------
Would be nice to have a version for ELF specifically that treated '0' and '1' as equal. 

================
Comment at: llvm/include/llvm/Support/Alignment.h:373
+}
+inline bool operator<=(const MaybeAlign &Lhs, uint64_t Rhs) {
+  ALIGN_CHECK_ISSET(Lhs);
----------------
Ditto about ELF specific option. Not sure how other parts of the code treat '0' so this might be a follow up.

================
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);
----------------
How does `Optional<T>` handle equality in the `None` case? I think we should match those semantics....maybe depends on what they are.

================
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; }
+
----------------
Won't `std::max` do this automatically since you implemented the comparison operators?

================
Comment at: llvm/include/llvm/Support/Alignment.h:518-528
+inline MaybeAlign max(const MaybeAlign &A, const MaybeAlign &B) {
+  return MaybeAlign(std::max(A.rawValue(), B.rawValue()));
+}
+
+inline Align max(const Align &A, const MaybeAlign &B) {
+  return Align(std::max(A.value(), B.rawValue()));
+}
----------------
These are again consistent with how ELF treats 0 since `0 <= x` for all alignments `x` in ELF but some motivation for how these are used is required IMO since it isn't a priori clear what these should do.

================
Comment at: llvm/include/llvm/Support/Alignment.h:552-562
+inline MaybeAlign min(const MaybeAlign &A, const MaybeAlign &B) {
+  return MaybeAlign(std::min(A.rawValue(), B.rawValue()));
+}
+
+inline MaybeAlign min(const Align &A, const MaybeAlign &B) {
+  return MaybeAlign(std::min(A.value(), B.rawValue()));
+}
----------------
ditto on justification for these.

================
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*/};
+}
----------------
These values are wrong and not powers of two.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64790