[AggressiveInstCombine] Fix cases where non-opaque pointers are used

[dstuttard]

Summary

In the case of non-opaque pointers, when combining consecutive loads,
need to bitcast the pointer source to the combined type size, otherwise
asserts are triggered.

[nikic]

Please remove the typed pointer test case. I'm willing to accept the patch at this time, but not the test case.

llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp

814

Checking for opaque pointers is not necessary, the bitcast will just not be emitted in that case.

817

You can sue getPointerAddressSpace() here and save the cast.

824

Extract the IntegerType into a variable for reuse. It is used a third time in the isTypeLegal check above.

[bipmis]

llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp

816

We already have the Pointer Address space defined for the Load Instruction. So the below should suffice

Type *NewTy = IntegerType::get(Load1Ptr->getContext(), LOps.LoadSize);
Value *NewPtr = Builder.CreateBitCast(Load1Ptr, NewTy->getPointerTo());
NewLoad = Builder.CreateAlignedLoad(NewTy, NewPtr, LI1->getAlign(),
                                    LI1->isVolatile(), "");

[dstuttard]

Thanks - removed the test and made the changes.

[dstuttard]

Updated for review requests

[nikic]

llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp

816

I don't think that's entirely correct, use still need to pass AS to getPointerTo().

[dstuttard]

Include address space AS in the pointerto call

[nikic]

LGTM

llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp

static bool foldConsecutiveLoads(Instruction &I, const DataLayout &DL,
   Allowed = TTI.allowsMisalignedMemoryAccesses(I.getContext(), LOps.LoadSize,
                                                AS, LI1->getAlign(), &Fast);
   if (!Allowed || !Fast)
     return false;
 
   // New load can be generated
   Value *Load1Ptr = LI1->getPointerOperand();
   Builder.SetInsertPoint(LI1);
+  if (!cast<PointerType>(Load1Ptr->getType())->isOpaque()) {
+    // We require an extra bitcast for non-opaque cases
+    unsigned Load1AS =
+        cast<PointerType>(Load1Ptr->getType())->getAddressSpace();
+    Load1Ptr = Builder.CreateBitCast(
+        Load1Ptr,
+        PointerType::get(
+            IntegerType::get(Load1Ptr->getContext(), LOps.LoadSize), Load1AS));
+  }
   NewLoad = Builder.CreateAlignedLoad(
       IntegerType::get(Load1Ptr->getContext(), LOps.LoadSize), Load1Ptr,
       LI1->getAlign(), LI1->isVolatile(), "");
   NewLoad->takeName(LI1);
   // Set the New Load AATags Metadata.
   if (LOps.AATags)
     NewLoad->setAAMetadata(LOps.AATags);
 
   Value *NewOp = NewLoad;
   // Check if zero extend needed.

llvm/test/Transforms/AggressiveInstCombine/X86/non-opaque-or-load.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -passes=aggressive-instcombine -mtriple x86_64-none-eabi -mattr=avx2 -data-layout="e-n64" -S | FileCheck %s --check-prefixes=LE
+; RUN: opt < %s -passes=aggressive-instcombine -mtriple x86_64-none-eabi -mattr=avx2 -data-layout="E-n64" -S | FileCheck %s --check-prefixes=BE
+
+define i16 @loadCombine_2consecutive(i8* %p) {
+;
+; LE-LABEL: @loadCombine_2consecutive(
+; LE-NEXT:    [[TMP1:%.*]] = bitcast i8* [[P:%.*]] to i16*
+; LE-NEXT:    [[L1:%.*]] = load i16, i16* [[TMP1]], align 1
+; LE-NEXT:    ret i16 [[L1]]
+;
+; BE-LABEL: @loadCombine_2consecutive(
+; BE-NEXT:    [[P1:%.*]] = getelementptr i8, i8* [[P:%.*]], i32 1
+; BE-NEXT:    [[L1:%.*]] = load i8, i8* [[P]], align 1
+; BE-NEXT:    [[L2:%.*]] = load i8, i8* [[P1]], align 1
+; BE-NEXT:    [[E1:%.*]] = zext i8 [[L1]] to i16
+; BE-NEXT:    [[E2:%.*]] = zext i8 [[L2]] to i16
+; BE-NEXT:    [[S2:%.*]] = shl i16 [[E2]], 8
+; BE-NEXT:    [[O1:%.*]] = or i16 [[E1]], [[S2]]
+; BE-NEXT:    ret i16 [[O1]]
+;
+  %p1 = getelementptr i8, i8* %p, i32 1
+  %l1 = load i8, i8* %p
+  %l2 = load i8, i8* %p1
+  %e1 = zext i8 %l1 to i16
+  %e2 = zext i8 %l2 to i16
+  %s2 = shl i16 %e2, 8
+  %o1 = or i16 %e1, %s2
+  ret i16 %o1
+}