Correct CreateAlignmentAssumption to check sign and power-of-2 (Clang Part)

[lebedev.ri]

Summary

CreateAlignmentAssumption with a run-time alignment value doesn't
correctly condition based on signedness, which can cause some odd
behaviors on large unsigneds. Additionally, only powers-of-two
are valid here, add that check to the assumption to prevent UB.

[erichkeane]

This comment was removed by erichkeane.

[lebedev.ri]

LG.
I hope that extra is-power-of-two won't confuse the optimizer.

[hfinkel]

As discussed on IRC, check sign/power of 2 correctly for the alignment assumptions.

"As discussed on IRC" is not appropriate for a commit message. The rationale must be documented here.

I hope that extra is-power-of-two won't confuse the optimizer.

Probably would unless it gets constant folded.

[lebedev.ri]

As disscussed in D54653 (https://reviews.llvm.org/D54653#1318964),
this differential will be simply unneeded, that review will address the issue too, so abandoning.

lib/CodeGen/CGCall.cpp

if (const auto *AA = TargetDecl->getAttr<AssumeAlignedAttr>()) {
       if (const auto *Offset = AA->getOffset())
         OffsetValue = EmitScalarExpr(Offset);
 
       llvm::Value *Alignment = EmitScalarExpr(AA->getAlignment());
       llvm::ConstantInt *AlignmentCI = cast<llvm::ConstantInt>(Alignment);
       EmitAlignmentAssumption(Ret.getScalarVal(), AlignmentCI->getZExtValue(),
                               OffsetValue);
     } else if (const auto *AA = TargetDecl->getAttr<AllocAlignAttr>()) {
-      llvm::Value *ParamVal =
-          CallArgs[AA->getParamIndex().getLLVMIndex()].getRValue(
-              *this).getScalarVal();
-      EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
+      CallArg Arg = CallArgs[AA->getParamIndex().getLLVMIndex()];
+      llvm::Value *ParamVal = Arg.getRValue(*this).getScalarVal();
+      EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal, *Arg.getType());
     }
   }
 
   return Ret;
 }
 
 CGCallee CGCallee::prepareConcreteCallee(CodeGenFunction &CGF) const {
   if (isVirtual()) {

lib/CodeGen/CodeGenFunction.h

   /// protection is no longer required.
   ///
   /// There's no particular reason why this shouldn't apply to
   /// l-values, it's just that no existing peepholes work on pointers.
   PeepholeProtection protectFromPeepholes(RValue rvalue);
   void unprotectFromPeepholes(PeepholeProtection protection);
 
   void EmitAlignmentAssumption(llvm::Value *PtrValue, llvm::Value *Alignment,
+                               const clang::Type &AlignmentType,
                                llvm::Value *OffsetValue = nullptr) {
-    Builder.CreateAlignmentAssumption(CGM.getDataLayout(), PtrValue, Alignment,
-                                      OffsetValue);
+    assert(AlignmentType.isIntegralOrUnscopedEnumerationType() &&
+           "Alignment can only be an integer type");
+    Builder.CreateAlignmentAssumption(
+        CGM.getDataLayout(), PtrValue, Alignment,
+        AlignmentType.isSignedIntegerOrEnumerationType()
+            ? llvm::AlignmentSign::Signed
+            : llvm::AlignmentSign::Unsigned,
   }
 
   //===--------------------------------------------------------------------===//
   //                             Statement Emission
   //===--------------------------------------------------------------------===//
 
   /// EmitStopPoint - Emit a debug stoppoint if we are emitting debug info.
   void EmitStopPoint(const Stmt *S);

test/CodeGen/alloc-align-attr.c

 // RUN: %clang_cc1 -triple x86_64-pc-linux -emit-llvm -o - %s | FileCheck %s
 
 __INT32_TYPE__*m1(__INT32_TYPE__ i) __attribute__((alloc_align(1)));
 
 // Condition where parameter to m1 is not size_t.
 __INT32_TYPE__ test1(__INT32_TYPE__ a) {
 // CHECK: define i32 @test1
   return *m1(a);
 // CHECK: call i32* @m1(i32 [[PARAM1:%[^\)]+]]) 
 // CHECK: [[ALIGNCAST1:%.+]] = sext i32 [[PARAM1]] to i64
-// CHECK: [[ISPOS1:%.+]] = icmp sgt i64 [[ALIGNCAST1]], 0
-// CHECK: [[POSMASK1:%.+]] = sub i64 [[ALIGNCAST1]], 1
-// CHECK: [[MASK1:%.+]] = select i1 [[ISPOS1]], i64 [[POSMASK1]], i64 0
+// CHECK: [[ISPOS:%.+]] = icmp sgt i64 [[ALIGNCAST1]], 0
+// CHECK: [[ALIGNSUB1:%.+]] = sub i64 [[ALIGNCAST1]], 1
+// CHECK: [[ALIGNANDSUB1:%.+]] = and i64 [[ALIGNCAST1]], [[ALIGNSUB1]]
+// CHECK: [[ISPOWER2:%.+]] = icmp eq i64 [[ALIGNANDSUB1]], 0
+// CHECK: [[ALIGNVALID:%.+]] = and i1 [[ISPOS]], [[ISPOWER2]]
+// CHECK: [[MASK1:%.+]] = select i1 [[ALIGNVALID]], i64 [[ALIGNSUB1]], i64 0
 // CHECK: [[PTRINT1:%.+]] = ptrtoint
 // CHECK: [[MASKEDPTR1:%.+]] = and i64 [[PTRINT1]], [[MASK1]]
 // CHECK: [[MASKCOND1:%.+]] = icmp eq i64 [[MASKEDPTR1]], 0
 // CHECK: call void @llvm.assume(i1 [[MASKCOND1]])
 }
 // Condition where test2 param needs casting.
 __INT32_TYPE__ test2(__SIZE_TYPE__ a) {
 // CHECK: define i32 @test2
   return *m1(a);
 // CHECK: [[CONV2:%.+]] = trunc i64 %{{.+}} to i32
 // CHECK: call i32* @m1(i32 [[CONV2]])
 // CHECK: [[ALIGNCAST2:%.+]] = sext i32 [[CONV2]] to i64
-// CHECK: [[ISPOS2:%.+]] = icmp sgt i64 [[ALIGNCAST2]], 0
-// CHECK: [[POSMASK2:%.+]] = sub i64 [[ALIGNCAST2]], 1
-// CHECK: [[MASK2:%.+]] = select i1 [[ISPOS2]], i64 [[POSMASK2]], i64 0
+// CHECK: [[ISPOS:%.+]] = icmp sgt i64 [[ALIGNCAST2]], 0
+// CHECK: [[ALIGNSUB1:%.+]] = sub i64 [[ALIGNCAST2]], 1
+// CHECK: [[ALIGNANDSUB1:%.+]] = and i64 [[ALIGNCAST2]], [[ALIGNSUB1]]
+// CHECK: [[ISPOWER2:%.+]] = icmp eq i64 [[ALIGNANDSUB1]], 0
+// CHECK: [[ALIGNVALID:%.+]] = and i1 [[ISPOS]], [[ISPOWER2]]
+// CHECK: [[MASK2:%.+]] = select i1 [[ALIGNVALID]], i64 [[ALIGNSUB1]], i64 0
 // CHECK: [[PTRINT2:%.+]] = ptrtoint
 // CHECK: [[MASKEDPTR2:%.+]] = and i64 [[PTRINT2]], [[MASK2]]
 // CHECK: [[MASKCOND2:%.+]] = icmp eq i64 [[MASKEDPTR2]], 0
 // CHECK: call void @llvm.assume(i1 [[MASKCOND2]])
 }
 __INT32_TYPE__ *m2(__SIZE_TYPE__ i) __attribute__((alloc_align(1)));
 
 // test3 param needs casting, but 'm2' is correct.
 __INT32_TYPE__ test3(__INT32_TYPE__ a) {
 // CHECK: define i32 @test3
   return *m2(a);
 // CHECK: [[CONV3:%.+]] = sext i32 %{{.+}} to i64
 // CHECK: call i32* @m2(i64 [[CONV3]])
-// CHECK: [[ISPOS3:%.+]] = icmp sgt i64 [[CONV3]], 0
-// CHECK: [[POSMASK3:%.+]] = sub i64 [[CONV3]], 1
-// CHECK: [[MASK3:%.+]] = select i1 [[ISPOS3]], i64 [[POSMASK3]], i64 0
+// CHECK: [[ISPOS:%.+]] = icmp ugt i64 [[CONV3]], 0
+// CHECK: [[ALIGNSUB1:%.+]] = sub i64 [[CONV3]], 1
+// CHECK: [[ALIGNANDSUB1:%.+]] = and i64 [[CONV3]], [[ALIGNSUB1]]
+// CHECK: [[ISPOWER2:%.+]] = icmp eq i64 [[ALIGNANDSUB1]], 0
+// CHECK: [[ALIGNVALID:%.+]] = and i1 [[ISPOS]], [[ISPOWER2]]
+// CHECK: [[MASK3:%.+]] = select i1 [[ALIGNVALID]], i64 [[ALIGNSUB1]], i64 0
 // CHECK: [[PTRINT3:%.+]] = ptrtoint
 // CHECK: [[MASKEDPTR3:%.+]] = and i64 [[PTRINT3]], [[MASK3]]
 // CHECK: [[MASKCOND3:%.+]] = icmp eq i64 [[MASKEDPTR3]], 0
 // CHECK: call void @llvm.assume(i1 [[MASKCOND3]])
 }
 
 // Every type matches, canonical example.
 __INT32_TYPE__ test4(__SIZE_TYPE__ a) {
 // CHECK: define i32 @test4
   return *m2(a);
 // CHECK: call i32* @m2(i64 [[PARAM4:%[^\)]+]]) 
-// CHECK: [[ISPOS4:%.+]] = icmp sgt i64 [[PARAM4]], 0
-// CHECK: [[POSMASK4:%.+]] = sub i64 [[PARAM4]], 1
-// CHECK: [[MASK4:%.+]] = select i1 [[ISPOS4]], i64 [[POSMASK4]], i64 0
+// CHECK: [[ISPOS:%.+]] = icmp ugt i64 [[PARAM4]], 0
+// CHECK: [[ALIGNSUB1:%.+]] = sub i64 [[PARAM4]], 1
+// CHECK: [[ALIGNANDSUB1:%.+]] = and i64 [[PARAM4]], [[ALIGNSUB1]]
+// CHECK: [[ISPOWER2:%.+]] = icmp eq i64 [[ALIGNANDSUB1]], 0
+// CHECK: [[ALIGNVALID:%.+]] = and i1 [[ISPOS]], [[ISPOWER2]]
+// CHECK: [[MASK4:%.+]] = select i1 [[ALIGNVALID]], i64 [[ALIGNSUB1]], i64 0
 // CHECK: [[PTRINT4:%.+]] = ptrtoint
 // CHECK: [[MASKEDPTR4:%.+]] = and i64 [[PTRINT4]], [[MASK4]]
 // CHECK: [[MASKCOND4:%.+]] = icmp eq i64 [[MASKEDPTR4]], 0
 // CHECK: call void @llvm.assume(i1 [[MASKCOND4]])
 }
 
-
 struct Empty {};
 struct MultiArgs { __INT64_TYPE__ a, b;};
 // Struct parameter doesn't take up an IR parameter, 'i' takes up 2.
 // Truncation to i64 is permissible, since alignments of greater than 2^64 are insane.
 __INT32_TYPE__ *m3(struct Empty s, __int128_t i) __attribute__((alloc_align(2)));
 __INT32_TYPE__ test5(__int128_t a) {
 // CHECK: define i32 @test5
   struct Empty e;
   return *m3(e, a);
 // CHECK: call i32* @m3(i64 %{{.*}}, i64 %{{.*}}) 
 // CHECK: [[ALIGNCAST5:%.+]] = trunc i128 %{{.*}} to i64
-// CHECK: [[ISPOS5:%.+]] = icmp sgt i64 [[ALIGNCAST5]], 0
-// CHECK: [[POSMASK5:%.+]] = sub i64 [[ALIGNCAST5]], 1
-// CHECK: [[MASK5:%.+]] = select i1 [[ISPOS5]], i64 [[POSMASK5]], i64 0
+// CHECK: [[ISPOS:%.+]] = icmp sgt i64 [[ALIGNCAST5]], 0
+// CHECK: [[ALIGNSUB1:%.+]] = sub i64 [[ALIGNCAST5]], 1
+// CHECK: [[ALIGNANDSUB1:%.+]] = and i64 [[ALIGNCAST5]], [[ALIGNSUB1]]
+// CHECK: [[ISPOWER2:%.+]] = icmp eq i64 [[ALIGNANDSUB1]], 0
+// CHECK: [[ALIGNVALID:%.+]] = and i1 [[ISPOS]], [[ISPOWER2]]
+// CHECK: [[MASK5:%.+]] = select i1 [[ALIGNVALID]], i64 [[ALIGNSUB1]], i64 0
 // CHECK: [[PTRINT5:%.+]] = ptrtoint
 // CHECK: [[MASKEDPTR5:%.+]] = and i64 [[PTRINT5]], [[MASK5]]
 // CHECK: [[MASKCOND5:%.+]] = icmp eq i64 [[MASKEDPTR5]], 0
 // CHECK: call void @llvm.assume(i1 [[MASKCOND5]])
 }
 // Struct parameter takes up 2 parameters, 'i' takes up 2.
 __INT32_TYPE__ *m4(struct MultiArgs s, __int128_t i) __attribute__((alloc_align(2)));
 __INT32_TYPE__ test6(__int128_t a) {
 // CHECK: define i32 @test6
   struct MultiArgs e;
   return *m4(e, a);
 // CHECK: call i32* @m4(i64 %{{.*}}, i64 %{{.*}}, i64 %{{.*}}) 
 // CHECK: [[ALIGNCAST6:%.+]] = trunc i128 %{{.*}} to i64
-// CHECK: [[ISPOS6:%.+]] = icmp sgt i64 [[ALIGNCAST6]], 0
-// CHECK: [[POSMASK6:%.+]] = sub i64 [[ALIGNCAST6]], 1
-// CHECK: [[MASK6:%.+]] = select i1 [[ISPOS6]], i64 [[POSMASK6]], i64 0
+// CHECK: [[ISPOS:%.+]] = icmp sgt i64 [[ALIGNCAST6]], 0
+// CHECK: [[ALIGNSUB1:%.+]] = sub i64 [[ALIGNCAST6]], 1
+// CHECK: [[ALIGNANDSUB1:%.+]] = and i64 [[ALIGNCAST6]], [[ALIGNSUB1]]
+// CHECK: [[ISPOWER2:%.+]] = icmp eq i64 [[ALIGNANDSUB1]], 0
+// CHECK: [[ALIGNVALID:%.+]] = and i1 [[ISPOS]], [[ISPOWER2]]
+// CHECK: [[MASK6:%.+]] = select i1 [[ALIGNVALID]], i64 [[ALIGNSUB1]], i64 0
 // CHECK: [[PTRINT6:%.+]] = ptrtoint
 // CHECK: [[MASKEDPTR6:%.+]] = and i64 [[PTRINT6]], [[MASK6]]
 // CHECK: [[MASKCOND6:%.+]] = icmp eq i64 [[MASKEDPTR6]], 0
 // CHECK: call void @llvm.assume(i1 [[MASKCOND6]])
 }