set the alignment of mlir::AttributeStorage to 64 bit explicitly to fix 32 bit platform

[daquexian]

Summary

On some platform (like WebAssembly), alignof(mlir::AttributeStorage) is 4 instead of 8. As a result, it makes the program crashes since PointerLikeTypeTraits<mlir::Attribute>::NumLowBitsAvailable is 3.

So I explicitly set the alignment of mlir::AttributeStoarge to 64 bits, and set PointerLikeTypeTraits<mlir::Attribute>::NumLowBitsAvailable according to it.

I also fixed an another related error (alignof(NamedAttribute) -> alignof(DictionaryAttributeStorage)) based on reviewer's comments.

[rriddle]

mlir/include/mlir/IR/Attributes.h

1717–1718

We should explicitly set the alignment on the Attribute class to the 64 bit alignment instead.

[rriddle]

I'm quite surprised you only hit this one. Not supporting 32 bit is a known issue(I don't have a 32 bit machine), but should just be a series of setting the alignment properly on pointer like classes that we pack bits into.

[rriddle]

mlir/include/mlir/IR/Attributes.h

1717–1718

(Should clarify that I mean whatever attribute storage class is causing this to fail)

[daquexian]

Update according to review comments

[daquexian]

mlir/include/mlir/IR/Attributes.h

1717–1718

Thanks for your quick reply! I have updated the patch.

[dblaikie]

mlir/include/mlir/IR/Attributes.h

1717–1718

In this case, I think it'd be good to avoid hardcoding the num low bits, and depend on the alignment of the type, then, rather than trying to keep both in sync independently. (perhaps with a static_assert that the num low bits is at least 3 or whatever is required/used by other parts of mlir, for instance)

[daquexian]

mlir/include/mlir/IR/Attributes.h

1717–1718

The best way is "static_assert(alignof(mlir::Attribute) >= exp2(NumLowBitsAvailable));", but exp2 (or pow) is not a constexpr function. Do you have some advice? Thanks!

[dblaikie]

mlir/include/mlir/IR/Attributes.h

1717–1718

My preference would be for the code to define the NumLowBuitsAvailable in terms of the alignment, then assert the NumLowBitsAvailable is at least whatever is needed - rather than having the code still have two independent sources of truth and an assert that ties them together.

As for how to get NumLowBitsAvailable defined in terms of the alignment - I'm surprised to see that's not done anywhere else, so maybe my whole suggestion's a bit off-the-beaten-path.

I'd guess maybe writing a small constexpr function that computes the log2 to use for the NumLowBitsAvailable might be adequate?

https://godbolt.org/z/fzd3Tc - could be improved a bit to fail to build if called in a constexpr context with a non-power-of-two value.

[daquexian]

mlir/include/mlir/IR/Attributes.h

1717–1718

Your advice is great! Which file should this constexpr log2 function be placed?

[rriddle]

mlir/include/mlir/IR/Attributes.h

1717–1718

If you haven't seen it: https://github.com/llvm/llvm-project/blob/892605b449f8375c62227df2eac2b8f1d7180af6/llvm/include/llvm/Support/MathExtras.h#L577

We should be able to set NumLowBitsAvailable here to llvm::PointerLikeTypeTraits<mlir::AttributeStorage *>::NumLowBits. That class(mlir::AttributeStorage) should have its alignment set to 8. That is what I meant when I said we should mark alignment of the class, because realistically the alignment of mlir::Attribute shouldn't matter given that it isn't the thing we are taking the address of. Apologies for the terse response before, I was on mobile.

[daquexian]

mlir/include/mlir/IR/Attributes.h

1717–1718

realistically the alignment of mlir::Attribute shouldn't matter given that it isn't the thing we are taking the address of.

Thanks for your reply! But I don't understand here. In the implementation of AttributeStorage (e.g., https://github.com/llvm/llvm-project/blob/master/mlir/lib/IR/AttributeDetail.h#L87), the alignment of mlir::Attribute is used as the alignment of the allocated memory. Am I missing something?

[rriddle]

mlir/include/mlir/IR/Attributes.h

1717–1718

Ahhh, that doesn't look right. I would expect that to be alignof(DictionaryAttributeStorage).

[daquexian]

Set the alignment of AttributeStorage instead of Attribute.
Set PointerLikeTypeTraits<mlir::Attribute>::NumLowBitsAvailable in terms of alignment.
alignof(NamedAttribute) -> alignof(DictionaryAttributeStorage)

[daquexian]

mlir/include/mlir/IR/Attributes.h

1717–1718

Thanks! I just updated the patch to replace alignof(NamedAttribute) with alignof(DictionaryAttributeStorage), set the alignment of AttributeStorage and then set NumLowBitsAvailable in terms of this alignment.

[rriddle]

mlir/include/mlir/IR/Attributes.h

1717–1718

Can you update to use llvm::PointerLikeTypeTraits<mlir::AttributeStorage *>::NumLowBits instead of the log2 here?

[daquexian]

address comments

mlir/include/mlir/IR/Attributes.h

1717–1718

Thanks! I have updated it.

[daquexian]

@rriddle @dblaikie Is this patch ready to merge? Thanks!

[dblaikie]

In D91062#2398626, @daquexian wrote:

@rriddle @dblaikie Is this patch ready to merge? Thanks!

Looks like it - do you have commit access, or do you need someone else to commit it for you?

[daquexian]

In D91062#2401022, @dblaikie wrote:

In D91062#2398626, @daquexian wrote:

@rriddle @dblaikie Is this patch ready to merge? Thanks!

Looks like it - do you have commit access, or do you need someone else to commit it for you?

I don't have commit access. I will be appreciated if you or someone else can commit it on my behalf :)

mlir/include/mlir/IR/AttributeSupport.h

 //===----------------------------------------------------------------------===//
 
 namespace detail {
 class AttributeUniquer;
 } // end namespace detail
 
 /// Base storage class appearing in an attribute. Derived storage classes should
 /// only be constructed within the context of the AttributeUniquer.
-class AttributeStorage : public StorageUniquer::BaseStorage {
+class alignas(8) AttributeStorage : public StorageUniquer::BaseStorage {
   friend detail::AttributeUniquer;
   friend StorageUniquer;
 
 public:
   /// Get the type of this attribute.
   Type getType() const;
 
   /// Return the abstract descriptor for this attribute.

mlir/include/mlir/IR/Attributes.h

 /// Allow LLVM to steal the low bits of Attributes.
 template <> struct PointerLikeTypeTraits<mlir::Attribute> {
   static inline void *getAsVoidPointer(mlir::Attribute attr) {
     return const_cast<void *>(attr.getAsOpaquePointer());
   }
   static inline mlir::Attribute getFromVoidPointer(void *ptr) {
     return mlir::Attribute::getFromOpaquePointer(ptr);
   }
-  static constexpr int NumLowBitsAvailable = 3;
+  static constexpr int NumLowBitsAvailable = llvm::PointerLikeTypeTraits<
+      mlir::AttributeStorage *>::NumLowBitsAvailable;
 };
 
 template <>
 struct PointerLikeTypeTraits<mlir::SymbolRefAttr>
     : public PointerLikeTypeTraits<mlir::Attribute> {
   static inline mlir::SymbolRefAttr getFromVoidPointer(void *ptr) {
     return PointerLikeTypeTraits<mlir::Attribute>::getFromVoidPointer(ptr)
         .cast<mlir::SymbolRefAttr>();
   }
 };
 
 } // namespace llvm
 
 #endif

mlir/lib/IR/AttributeDetail.h

struct DictionaryAttributeStorage final
   /// Key equality function.
   bool operator==(const KeyTy &key) const { return key == getElements(); }
 
   /// Construct a new storage instance.
   static DictionaryAttributeStorage *
   construct(AttributeStorageAllocator &allocator, const KeyTy &key) {
     auto size = DictionaryAttributeStorage::totalSizeToAlloc<NamedAttribute>(
         key.size());
-    auto rawMem = allocator.allocate(size, alignof(NamedAttribute));
+    auto rawMem = allocator.allocate(size, alignof(DictionaryAttributeStorage));
 
     // Initialize the storage and trailing attribute list.
     auto result = ::new (rawMem) DictionaryAttributeStorage(key.size());
     std::uninitialized_copy(key.begin(), key.end(),
                             result->getTrailingObjects<NamedAttribute>());
     return result;
   }