[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/include/clang/Sema/Sema.h

bool shouldLinkPossiblyHiddenDecl(const NamedDecl *Old,
     return false;
   }
   bool shouldLinkPossiblyHiddenDecl(LookupResult &Old, const NamedDecl *New);
 
   void setupImplicitSpecialMemberType(CXXMethodDecl *SpecialMem,
                                       QualType ResultTy,
                                       ArrayRef<QualType> Args);
 
+  /// The maximum alignment, same as in llvm::Value. We duplicate them here
+  /// because that allows us not to duplicate the constants in clang code,
+  /// which we must to since we can't directly use the llvm constants.
+  ///
+  /// This is the greatest alignment value supported by load, store, and alloca
+  /// instructions, and global values.
+  static const unsigned MaxAlignmentExponent = 29;
+  static const unsigned MaximumAlignment = 1u << MaxAlignmentExponent;
+
 public:
   typedef OpaquePtr<DeclGroupRef> DeclGroupPtrTy;
   typedef OpaquePtr<TemplateName> TemplateTy;
   typedef OpaquePtr<QualType> TypeTy;
 
   OpenCLOptions OpenCLFeatures;
   FPOptions FPFeatures;
 

clang/lib/Sema/SemaChecking.cpp

if (!Arg->isValueDependent()) {
       llvm::APSInt I(64);
       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;
-        if (I > MaximumAlignment)
+        if (I > Sema::MaximumAlignment)
           Diag(Arg->getExprLoc(), diag::warn_assume_aligned_too_great)
-              << Arg->getSourceRange() << MaximumAlignment;
+              << Arg->getSourceRange() << Sema::MaximumAlignment;
       }
     }
   }
 
   if (FD)
     diagnoseArgDependentDiagnoseIfAttrs(FD, ThisArg, Args, Loc);
 }
 
if (!Arg->isTypeDependent() && !Arg->isValueDependent()) {
     llvm::APSInt Result;
     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;
-    if (Result > MaximumAlignment)
+    if (Result > Sema::MaximumAlignment)
       Diag(TheCall->getBeginLoc(), diag::warn_assume_aligned_too_great)
-          << Arg->getSourceRange() << MaximumAlignment;
+          << Arg->getSourceRange() << Sema::MaximumAlignment;
   }
 
   if (NumArgs > 2) {
     ExprResult Arg(TheCall->getArg(2));
     InitializedEntity Entity = InitializedEntity::InitializeParameter(Context,
       Context.getSizeType(), false);
     Arg = PerformCopyInitialization(Entity, SourceLocation(), Arg);
     if (Arg.isInvalid()) return true;

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;
-    if (I > MaximumAlignment)
+    if (I > Sema::MaximumAlignment)
       Diag(CI.getLoc(), diag::warn_assume_aligned_too_great)
-          << CI.getRange() << MaximumAlignment;
+          << CI.getRange() << Sema::MaximumAlignment;
   }
 
   if (OE) {
     if (!OE->isValueDependent()) {
       llvm::APSInt I(64);
       if (!OE->isIntegerConstantExpr(I, Context)) {
         Diag(AttrLoc, diag::err_attribute_argument_n_type)
           << &TmpAttr << 2 << AANT_ArgumentIntegerConstant
void Sema::AddAlignedAttr(Decl *D, const AttributeCommonInfo &CI, Expr *E,
   if (!(TmpAttr.isAlignas() && !Alignment)) {
     if (!llvm::isPowerOf2_64(AlignVal)) {
       Diag(AttrLoc, diag::err_alignment_not_power_of_two)
         << E->getSourceRange();
       return;
     }
   }
 
-  // Alignment calculations can wrap around if it's greater than 2**28.
-  unsigned MaxValidAlignment =
-      Context.getTargetInfo().getTriple().isOSBinFormatCOFF() ? 8192
-                                                              : 268435456;
-  if (AlignVal > MaxValidAlignment) {
-    Diag(AttrLoc, diag::err_attribute_aligned_too_great) << MaxValidAlignment
-                                                         << E->getSourceRange();
+  if (AlignVal > Sema::MaximumAlignment) {
+    Diag(AttrLoc, diag::err_attribute_aligned_too_great)
+        << Sema::MaximumAlignment << E->getSourceRange();
     return;
   }
 
   if (Context.getTargetInfo().isTLSSupported()) {
     unsigned MaxTLSAlign =
         Context.toCharUnitsFromBits(Context.getTargetInfo().getMaxTLSAlign())
             .getQuantity();
     const auto *VD = dyn_cast<VarDecl>(D);

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.