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

[lebedev.ri]

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.

[hfinkel]

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.

[lebedev.ri]

In D72998#1828559, @hfinkel wrote:

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.

[erichkeane]

In D72998#1828571, @lebedev.ri wrote:

In D72998#1828559, @hfinkel wrote:

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

[lebedev.ri]

In D72998#1831377, @erichkeane wrote:

In D72998#1828571, @lebedev.ri wrote:

In D72998#1828559, @hfinkel wrote:

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

[lebedev.ri]

Add constants to Sema class.

[gchatelet]

LGTM

[lebedev.ri]

Thank you for the reviews!

[lebedev.ri]

Rebased, NFC.

[davezarzycki]

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.

[lebedev.ri]

In D72998#1838760, @davezarzycki wrote:

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!

[gchatelet]

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;
};

[lebedev.ri]

In D72998#1838811, @gchatelet wrote:

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.

[gchatelet]

In D72998#1838834, @lebedev.ri wrote:

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

clang/lib/Sema/SemaChecking.cpp

if (!Arg->isTypeDependent() && !Arg->isValueDependent()) {
       if (Arg->isIntegerConstantExpr(I, Context)) {
         if (!I.isPowerOf2()) {
           Diag(Arg->getExprLoc(), diag::err_alignment_not_power_of_two)
               << Arg->getSourceRange();
           return;
         }
 
         // Alignment calculations can wrap around if it's greater than 2**29.
-        unsigned MaximumAlignment = 536870912;
+        unsigned MaximumAlignment =
+            536870912; // See llvm::Value::MaximumAlignment
         if (I > MaximumAlignment)
           Diag(Arg->getExprLoc(), diag::warn_assume_aligned_too_great)
               << Arg->getSourceRange() << MaximumAlignment;
       }
     }
   }
 
   if (FD)
if (!Arg->isTypeDependent() && !Arg->isValueDependent()) {
     if (SemaBuiltinConstantArg(TheCall, 1, Result))
       return true;
 
     if (!Result.isPowerOf2())
       return Diag(TheCall->getBeginLoc(), diag::err_alignment_not_power_of_two)
              << Arg->getSourceRange();
 
     // Alignment calculations can wrap around if it's greater than 2**29.
-    unsigned MaximumAlignment = 536870912;
+    unsigned MaximumAlignment = 536870912; // See llvm::Value::MaximumAlignment
     if (Result > MaximumAlignment)
       Diag(TheCall->getBeginLoc(), diag::warn_assume_aligned_too_great)
           << Arg->getSourceRange() << MaximumAlignment;
   }
 
   if (NumArgs > 2) {
     ExprResult Arg(TheCall->getArg(2));
     InitializedEntity Entity = InitializedEntity::InitializeParameter(Context,

clang/lib/Sema/SemaDeclAttr.cpp

if (!E->isValueDependent()) {
 
     if (!I.isPowerOf2()) {
       Diag(AttrLoc, diag::err_alignment_not_power_of_two)
         << E->getSourceRange();
       return;
     }
 
     // Alignment calculations can wrap around if it's greater than 2**29.
-    unsigned MaximumAlignment = 536870912;
+    unsigned MaximumAlignment = 536870912; // See llvm::Value::MaximumAlignment
     if (I > MaximumAlignment)
       Diag(CI.getLoc(), diag::warn_assume_aligned_too_great)
           << CI.getRange() << MaximumAlignment;
   }
 
   if (OE) {
     if (!OE->isValueDependent()) {
       llvm::APSInt I(64);

llvm/lib/IR/Attributes.cpp

if (!PA) {
     pImpl->AttrsSet.InsertNode(PA, InsertPoint);
   }
 
   // Return the Attribute that we found or created.
   return Attribute(PA);
 }
 
 Attribute Attribute::getWithAlignment(LLVMContext &Context, Align A) {
-  assert(A <= 0x40000000 && "Alignment too large.");
+  assert(A <= llvm::Value::MaximumAlignment && "Alignment too large.");
   return get(Context, Alignment, A.value());
 }
 
 Attribute Attribute::getWithStackAlignment(LLVMContext &Context, Align A) {
   assert(A <= 0x100 && "Alignment too large.");
   return get(Context, StackAlignment, A.value());
 }
 

 std::pair<unsigned, Optional<unsigned>> AttrBuilder::getAllocSizeArgs() const {
   return unpackAllocSizeArgs(AllocSizeArgs);
 }
 
 AttrBuilder &AttrBuilder::addAlignmentAttr(MaybeAlign Align) {
   if (!Align)
     return *this;
 
-  assert(*Align <= 0x40000000 && "Alignment too large.");
+  assert(*Align <= llvm::Value::MaximumAlignment && "Alignment too large.");
 
   Attrs[Attribute::Alignment] = true;
   Alignment = Align;
   return *this;
 }
 
 AttrBuilder &AttrBuilder::addStackAlignmentAttr(MaybeAlign Align) {
   // Default alignment, allow the target to define how to align it.