This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by daquexian on Nov 9 2020, 2:41 AM.

Details

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.

Diff Detail

Event Timeline

daquexian created this revision.Nov 9 2020, 2:41 AM
daquexian requested review of this revision.Nov 9 2020, 2:41 AM
rriddle requested changes to this revision.Nov 9 2020, 2:46 AM
rriddle added inline comments.
mlir/include/mlir/IR/Attributes.h
1702–1703

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

This revision now requires changes to proceed.Nov 9 2020, 2:46 AM

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 added inline comments.Nov 9 2020, 2:56 AM
mlir/include/mlir/IR/Attributes.h
1702–1703

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

daquexian updated this revision to Diff 303809.Nov 9 2020, 3:15 AM

Update according to review comments

daquexian marked an inline comment as done.Nov 9 2020, 3:17 AM
daquexian added inline comments.
mlir/include/mlir/IR/Attributes.h
1702–1703

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

daquexian edited the summary of this revision. (Show Details)Nov 9 2020, 3:27 AM
daquexian retitled this revision from Set PointerLikeTypeTraits<mlir::Attribute>::NumLowBitsAvailable according to the value of alignof(mlir::Attribute) to set the alignment of mlir::Attribute to 64 bit explicitly to fix 32 bit platform.Nov 9 2020, 6:26 AM
dblaikie added inline comments.Nov 9 2020, 4:55 PM
mlir/include/mlir/IR/Attributes.h
1702–1703

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 added inline comments.Nov 9 2020, 6:35 PM
mlir/include/mlir/IR/Attributes.h
1702–1703

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 added inline comments.Nov 9 2020, 7:10 PM
mlir/include/mlir/IR/Attributes.h
1702–1703

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 added inline comments.Nov 9 2020, 10:04 PM
mlir/include/mlir/IR/Attributes.h
1702–1703

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

rriddle added inline comments.Nov 9 2020, 10:14 PM
mlir/include/mlir/IR/Attributes.h
1702–1703

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 added inline comments.Nov 9 2020, 11:13 PM
mlir/include/mlir/IR/Attributes.h
1702–1703

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 added inline comments.Nov 9 2020, 11:18 PM
mlir/include/mlir/IR/Attributes.h
1702–1703

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

daquexian updated this revision to Diff 304078.EditedNov 10 2020, 1:10 AM
daquexian retitled this revision from set the alignment of mlir::Attribute to 64 bit explicitly to fix 32 bit platform to set the alignment of mlir::AttributeStorage to 64 bit explicitly to fix 32 bit platform.
daquexian edited the summary of this revision. (Show Details)

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

daquexian added inline comments.Nov 10 2020, 1:14 AM
mlir/include/mlir/IR/Attributes.h
1702–1703

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 added inline comments.Nov 10 2020, 10:48 PM
mlir/include/mlir/IR/Attributes.h
1702–1703

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

rriddle accepted this revision.Nov 10 2020, 10:48 PM
This revision is now accepted and ready to land.Nov 10 2020, 10:48 PM
daquexian updated this revision to Diff 304449.Nov 11 2020, 3:13 AM

address comments

mlir/include/mlir/IR/Attributes.h
1702–1703

Thanks! I have updated it.

dblaikie accepted this revision.Nov 11 2020, 12:50 PM

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

@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?

@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 :)