[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

813–814

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

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.

[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,
                                  TargetTransformInfo &TTI, AliasAnalysis &AA) {
   LoadOps LOps;
   if (!foldLoadsRecursive(&I, LOps, DL, AA) || !LOps.FoundRoot)
     return false;
 
   IRBuilder<> Builder(&I);
   LoadInst *NewLoad = nullptr, *LI1 = LOps.Root;
 
+  IntegerType *WiderType = IntegerType::get(I.getContext(), LOps.LoadSize);
   // TTI based checks if we want to proceed with wider load
-  bool Allowed =
-      TTI.isTypeLegal(IntegerType::get(I.getContext(), LOps.LoadSize));
+  bool Allowed = TTI.isTypeLegal(WiderType);
   if (!Allowed)
     return false;
 
   unsigned AS = LI1->getPointerAddressSpace();
   bool Fast = false;
   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);
-  NewLoad = Builder.CreateAlignedLoad(
-      IntegerType::get(Load1Ptr->getContext(), LOps.LoadSize), Load1Ptr,
-      LI1->getAlign(), LI1->isVolatile(), "");
+  Value *NewPtr = Builder.CreateBitCast(Load1Ptr, WiderType->getPointerTo(AS));
+  NewLoad = Builder.CreateAlignedLoad(WiderType, NewPtr, 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.
   if (LOps.ZextType)
     NewOp = Builder.CreateZExt(NewOp, LOps.ZextType);