InstCombineCalls: when adding an align attribute, never reduce it

[durin42]

Summary

Sometimes we can infer an align from an allocalign but the function
already promised it'd be more-aligned than the allocalign and there's an
existing align that we shouldn't reduce. Make sure we handle that
correctly.

[nikic]

llvm/test/Transforms/InstCombine/InferAlignAttribute.ll

2

We shouldn't be testing -O2 in InstCombine tests.

[jyknight]

llvm/test/Transforms/InstCombine/InferAlignAttribute.ll

37–39

Please explicitly note that this is mildly undesirable (yet conservatively correct) behavior. We would prefer the result to have an align 128, if that were to happen.

Also would be helpful to note that it is a simplified example of what happens in practice with _mm_malloc which calls posix_memalign.

[nikic]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

2843

While you're here: This explicit removeRetAttr shouldn't be necessary. addRetAttr overwrites an existing attribute.

[durin42]

removed the removeRetAttr - I agree it shouldn't be needed

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

void InstCombinerImpl::annotateAnyAllocSite(CallBase &Call, const TargetLibraryInfo *TLI) {
   Value *Alignment = getAllocAlignment(&Call, TLI);
   if (!Alignment)
     return;
 
   ConstantInt *AlignOpC = dyn_cast<ConstantInt>(Alignment);
   if (AlignOpC && AlignOpC->getValue().ult(llvm::Value::MaximumAlignment)) {
     uint64_t AlignmentVal = AlignOpC->getZExtValue();
     if (llvm::isPowerOf2_64(AlignmentVal)) {
-      Call.removeRetAttr(Attribute::Alignment);
-      Call.addRetAttr(Attribute::getWithAlignment(Call.getContext(),
-                                                  Align(AlignmentVal)));
+      Align ExistingAlign = Call.getRetAlign().valueOrOne();
+      Align NewAlign = Align(AlignmentVal);
+      if (NewAlign > ExistingAlign) {
+        Call.addRetAttr(
+            Attribute::getWithAlignment(Call.getContext(), NewAlign));
+      }
     }
   }
 }
 
 /// Improvements for call, callbr and invoke instructions.
 Instruction *InstCombinerImpl::visitCallBase(CallBase &Call) {
   annotateAnyAllocSite(Call, &TLI);
 

llvm/test/Transforms/InstCombine/InferAlignAttribute.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -S -inline -instcombine %s | FileCheck %s --check-prefixes=CHECK,CHECK-INLINE
 ; RUN: opt -S -instcombine %s | FileCheck %s --check-prefixes=CHECK,CHECK-NOINLINE
 
 define i8* @widen_align_from_allocalign_callsite() {
 ; CHECK-LABEL: @widen_align_from_allocalign_callsite(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[CALL:%.*]] = tail call align 64 i8* @my_aligned_alloc_2(i32 noundef 320, i32 allocalign noundef 64)
 ; CHECK-NEXT:    ret i8* [[CALL]]
 ;

 ; CHECK-NEXT:    ret i8* [[CALL]]
 ;
 entry:
 
   %call = tail call align 16 i8* @my_aligned_alloc(i32 noundef 320, i32 noundef 64)
   ret i8* %call
 }
 
-; BUG: we shouldn't narrow this alignment since we already had a stronger
-; constraint, but we do.
 define i8* @dont_narrow_align_from_allocalign() {
 ; CHECK-LABEL: @dont_narrow_align_from_allocalign(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[CALL:%.*]] = tail call align 8 i8* @my_aligned_alloc(i32 noundef 320, i32 noundef 8)
+; CHECK-NEXT:    [[CALL:%.*]] = tail call align 16 i8* @my_aligned_alloc(i32 noundef 320, i32 noundef 8)
 ; CHECK-NEXT:    ret i8* [[CALL]]
 ;
 entry:
   %call = tail call align 16 i8* @my_aligned_alloc(i32 noundef 320, i32 noundef 8)
   ret i8* %call
 }
 
 define i8* @my_aligned_alloc_3(i32 noundef %foo, i32 allocalign %alignment) {
 ; CHECK-LABEL: @my_aligned_alloc_3(
 ; CHECK-NEXT:    [[CALL:%.*]] = tail call i8* @my_aligned_alloc_2(i32 noundef [[FOO:%.*]], i32 noundef [[ALIGNMENT:%.*]])
 ; CHECK-NEXT:    ret i8* [[CALL]]
 ;
   %call = tail call i8* @my_aligned_alloc_2(i32 noundef %foo, i32 noundef %alignment)
   ret i8* %call
 }