[InstCombine] Let combineLoadToNewType preserve ABI alignment of the load (PR44543)

[aqjune]

Summary

If aligment on LoadInst isn't specified, load is assumed to be ABI-aligned.
And said aligment may be different for different types.
So if we change load type, but don't pay extra attention to the aligment
(i.e. keep it unspecified), we may either overpromise (if the default aligment
of the new type is higher), or underpromise (if the default aligment
of the new type is smaller).

Thus, if no alignment is specified, we need to manually preserve the implied ABI alignment.

This addresses https://bugs.llvm.org/show_bug.cgi?id=44543 by making combineLoadToNewType preserve ABI alignment of the load.

[aqjune]

Minor update

[lebedev.ri]

llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

465

Align

466

// If old load did not have an explicit alignment specified,
// manually preserve the implied (ABI) alignment of the load.
// Else we may inadvertently incorrectly over-promise alignment.

llvm/test/Transforms/InstCombine/load-bitcast64.ll

4

Please precommit the test changes first, and please use the script to autogenerate check lines

[aqjune]

Address comments

[aqjune]

llvm/test/Transforms/InstCombine/load-bitcast64.ll

4

Ok, I'll make a separate commit for this.

[aqjune]

llvm/test/Transforms/InstCombine/load-bitcast64.ll

4

Or do you prefer having the auto generated check in this patch?

[lebedev.ri]

llvm/test/Transforms/InstCombine/load-bitcast64.ll

4

First, change the datalayout and autogenerate the check lines here, and commit that.
Then, rebase this patch to only show the changes caused by the fix itself.

[aqjune]

https://reviews.llvm.org/rG0877843ddacc applied, update diff from master

[lebedev.ri]

LG, thank you.

llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

466

Please do mark done comments as done. (the checkbox)

llvm/test/Transforms/InstCombine/load-bitcast64.ll

60

Nice

[aqjune]

Just noticed that I landed this without explicit accept; sorry.
PR44543 is now resolved.

[lebedev.ri]

In D72710#1820102, @aqjune wrote:

Just noticed that I landed this without explicit accept; sorry.
PR44543 is now resolved.

(In this case i did accept it, it got lost on my side)

llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

static LoadInst *combineLoadToNewType(InstCombiner &IC, LoadInst &LI, Type *NewTy,
   Value *Ptr = LI.getPointerOperand();
   unsigned AS = LI.getPointerAddressSpace();
   Value *NewPtr = nullptr;
   if (!(match(Ptr, m_BitCast(m_Value(NewPtr))) &&
         NewPtr->getType()->getPointerElementType() == NewTy &&
         NewPtr->getType()->getPointerAddressSpace() == AS))
     NewPtr = IC.Builder.CreateBitCast(Ptr, NewTy->getPointerTo(AS));
 
+  unsigned Align = LI.getAlignment();
+  if (!Align)
+    // If old load did not have an explicit alignment specified,
+    // manually preserve the implied (ABI) alignment of the load.
+    // Else we may inadvertently incorrectly over-promise alignment.
+    Align = IC.getDataLayout().getABITypeAlignment(LI.getType());
+
   LoadInst *NewLoad = IC.Builder.CreateAlignedLoad(
-      NewTy, NewPtr, LI.getAlignment(), LI.isVolatile(), LI.getName() + Suffix);
+      NewTy, NewPtr, Align, LI.isVolatile(), LI.getName() + Suffix);
   NewLoad->setAtomic(LI.getOrdering(), LI.getSyncScopeID());
   copyMetadataForLoad(*NewLoad, LI);
   return NewLoad;
 }
 
 /// Combine a store to a new type.
 ///
 /// Returns the newly created store instruction.

llvm/test/Transforms/InstCombine/load-bitcast64.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -instcombine -S < %s | FileCheck %s
 
 target datalayout = "p:64:64:64-i64:32:32"
 
 
 define i64* @test1(i8* %x) {
 ; CHECK-LABEL: @test1(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[TMP0:%.*]] = bitcast i8* [[X:%.*]] to i64**
-; CHECK-NEXT:    [[B1:%.*]] = load i64*, i64** [[TMP0]], align 8
+; CHECK-NEXT:    [[B1:%.*]] = load i64*, i64** [[TMP0]], align 4
 ; CHECK-NEXT:    ret i64* [[B1]]
 ;
 entry:
   %a = bitcast i8* %x to i64*
   %b = load i64, i64* %a
   %c = inttoptr i64 %b to i64*
 
   ret i64* %c
entry:
 
   ret i64* %c
 }
 
 define i64 @test4(i8* %x) {
 ; CHECK-LABEL: @test4(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[TMP0:%.*]] = bitcast i8* [[X:%.*]] to i64*
-; CHECK-NEXT:    [[B1:%.*]] = load i64, i64* [[TMP0]], align 4
+; CHECK-NEXT:    [[B1:%.*]] = load i64, i64* [[TMP0]], align 8
 ; CHECK-NEXT:    ret i64 [[B1]]
 ;
 entry:
   %a = bitcast i8* %x to i64**
   %b = load i64*, i64** %a
   %c = ptrtoint i64* %b to i64
 
   ret i64 %c
entry:
 
   ret i32 %c
 }
 
 define i64 @test6(i8* %x) {
 ; CHECK-LABEL: @test6(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[TMP0:%.*]] = bitcast i8* [[X:%.*]] to i64*
-; CHECK-NEXT:    [[B1:%.*]] = load i64, i64* [[TMP0]], align 4
+; CHECK-NEXT:    [[B1:%.*]] = load i64, i64* [[TMP0]], align 8
 ; CHECK-NEXT:    ret i64 [[B1]]
 ;
 entry:
   %a = bitcast i8* %x to i32**
   %b = load i32*, i32** %a
   %c = ptrtoint i32* %b to i64
 
   ret i64 %c
 }