Page MenuHomePhabricator

[IR] Attribute/AttrBuilder: use Value::MaximumAlignment magic constant
ClosedPublic

Authored by lebedev.ri on Jan 19 2020, 5:42 AM.

Details

Summary

I initially encountered those assertions when trying to create
this IR alignment attribute from clang's __attribute__((assume_aligned(imm))),
because until D72994 there is no sanity checking for the value of imm.

But even then, we have llvm::Value::MaximumAlignment constant (which is 536870912),
which is enforced for clang attributes, and then there are some other magical constant
(0x40000000 i.e. 1073741824 i.e. 2 * 536870912) in
Attribute::getWithAlignment()/AttrBuilder::addAlignmentAttr().

I strongly suspect that 0x40000000 is incorrect,
and that also should be llvm::Value::MaximumAlignment.

Diff Detail

Event Timeline

lebedev.ri created this revision.Jan 19 2020, 5:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2020, 5:42 AM

Can we, at least, put this constant in a header file so we don't repeat it in several places? Also, can we write it as 0x20000000 so that it's more obvious what the value is.

Can we, at least, put this constant in a header file so we don't repeat it in several places?

Remember, we can't use llvm::Value::MaximumAlignment itself in clang sema.
Which header do you have in mind?

Also, can we write it as 0x20000000 so that it's more obvious what the value is.

Sure, will do.

Can we, at least, put this constant in a header file so we don't repeat it in several places?

Remember, we can't use llvm::Value::MaximumAlignment itself in clang sema.
Which header do you have in mind?

Also, can we write it as 0x20000000 so that it's more obvious what the value is.

Sure, will do.

I'd suggest just putting it as a constant in SEMA. Also can you fix this one too? https://github.com/llvm-mirror/clang/commit/726918e196b413bcd80f08494c061fd6b3f26c94

Can we, at least, put this constant in a header file so we don't repeat it in several places?

Remember, we can't use llvm::Value::MaximumAlignment itself in clang sema.
Which header do you have in mind?

Also, can we write it as 0x20000000 so that it's more obvious what the value is.

Sure, will do.

I'd suggest just putting it as a constant in SEMA. Also can you fix this one too? https://github.com/llvm-mirror/clang/commit/726918e196b413bcd80f08494c061fd6b3f26c94

Okay

Add constants to Sema class.

erichkeane accepted this revision.Jan 23 2020, 9:12 AM
This revision is now accepted and ready to land.Jan 23 2020, 9:12 AM

Thank you for the reviews!

Rebased, NFC.

This revision was automatically updated to reflect the committed changes.

I understand not wanting to have trivial dependencies on IR for values like this, but what about creating llvm/Support/InternalLimits.h to hold values like this, and then *either* have llvm::Value::MaximumAlignment derive from the constant in the new file, or static_assert that the internal limit constant equals llvm::Value::MaximumAlignment?

At the very least, something in clang's IR generation logic should static_assert that the Sema constant is less than or equal to the LLVM constant.

I understand not wanting to have trivial dependencies on IR for values like this, but what about creating llvm/Support/InternalLimits.h to hold values like this, and then *either* have llvm::Value::MaximumAlignment derive from the constant in the new file, or static_assert that the internal limit constant equals llvm::Value::MaximumAlignment?

At the very least, something in clang's IR generation logic should static_assert that the Sema constant is less than or equal to the LLVM constant.

I agree that adding such a sanity check may be a good next step,
although i don't have any great ideas where it should reside.
Feel free to submit a patch!

Actually there's an issue with the code. It doesn't compile in shared_library mode.

ld.lld: error: undefined symbol: clang::Sema::MaximumAlignment
>>> referenced by SemaChecking.cpp:5397 (/redacted/llvm-project/clang/lib/Sema/SemaChecking.cpp:5397)
>>>               tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaChecking.cpp.o:(clang::Sema::SemaBuiltinAssumeAligned(clang::CallExpr*))
>>> referenced by SemaChecking.cpp:3670 (/redacted/llvm-project/clang/lib/Sema/SemaChecking.cpp:3670)
>>>               tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaChecking.cpp.o:(clang::Sema::checkCall(clang::NamedDecl*, clang::FunctionProtoType const*, clang::Expr const*, llvm::ArrayRef<clang::Expr const*>, bool, clang::SourceLocation, clang::SourceRange, clang::Sema::VariadicCallType))
>>> referenced by SemaDeclAttr.cpp:1631 (/redacted/llvm-project/clang/lib/Sema/SemaDeclAttr.cpp:1631)
>>>               tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaDeclAttr.cpp.o:(clang::Sema::AddAssumeAlignedAttr(clang::Decl*, clang::AttributeCommonInfo const&, clang::Expr*, clang::Expr*))

It comes from the fact that Diag::operator<< takes arguments by const&.

You'd need to anchor the value in Sema.cpp but then the compiler doesn't see the value anymore, another option would be to use inlined variables (but this is C++17) or use an anonymous enum value.

enum {
  MaxAlignmentExponent = 29;
  MaximumAlignment = 1u << MaxAlignmentExponent;
};

Actually there's an issue with the code. It doesn't compile in shared_library mode.

 ld.lld: error: undefined symbol: clang::Sema::MaximumAlignment
>>> referenced by SemaChecking.cpp:5397 (/redacted/llvm-project/clang/lib/Sema/SemaChecking.cpp:5397)
>>>               tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaChecking.cpp.o:(clang::Sema::SemaBuiltinAssumeAligned(clang::CallExpr*))
>>> referenced by SemaChecking.cpp:3670 (/redacted/llvm-project/clang/lib/Sema/SemaChecking.cpp:3670)
>>>               tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaChecking.cpp.o:(clang::Sema::checkCall(clang::NamedDecl*, clang::FunctionProtoType const*, clang::Expr const*, llvm::ArrayRef<clang::Expr const*>, bool, clang::SourceLocation, clang::SourceRange, clang::Sema::VariadicCallType))
>>> referenced by SemaDeclAttr.cpp:1631 (/redacted/llvm-project/clang/lib/Sema/SemaDeclAttr.cpp:1631)
>>>               tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaDeclAttr.cpp.o:(clang::Sema::AddAssumeAlignedAttr(clang::Decl*, clang::AttributeCommonInfo const&, clang::Expr*, clang::Expr*))

Is that still an issue after a8c3608a27a82cf1c66f33b96a06423fe0e708fc ?
http://lab.llvm.org:8011/builders/clang-armv7-linux-build-cache gone back to green after that.

Not an issue anymore! Thx for the fix :)