Unpack array of all sizes in InstCombine

[deadalnix]

Summary

This is another step toward improving fca support. This unpack load of array in a series of load to array's elements.

[deadalnix]

Ping ?

[deadalnix]

Ping !

[deadalnix]

Ping ?

[deadalnix]

ping

[mgrang]

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

526

Why can't we simply call LI.getName() wherever it is required rather than maintaining the variable "Name"?

531–532

Same here. Do we really need to maintain NumElements? Can we simply call ST->getNumElements() instead?

582

I think these could be written better as:

SmallString<16> LoadName = LI->getName() + ".unpack";
SmallString<16> EltName = LI->getName() + ".elt";

[deadalnix]

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

526

Is that a

526

Is that a problem ? I generally avoid calling the same thing again and again. In this case, it probably doesn't matter much.

582

error: no viable conversion from 'llvm::Twine' to 'SmallString<16>'

[deadalnix]

ping ?

[deadalnix]

Fix alignement on generated loads.

[deadalnix]

clang format

[deadalnix]

Ping ?

[deadalnix]

ping

[majnemer]

Does this do the right thing with wacky stuff like arrays of i3?

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

611

Arrays can have more than 2**32 elements.

616

You should use uint64_t here.

[deadalnix]

Use 64 bits indices

[majnemer]

LGTM

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

static Instruction *unpackLoadToAggregate(InstCombiner &IC, LoadInst &LI) {
   // stores here but it isn't clear that this is important.
   if (!LI.isSimple())
     return nullptr;
 
   Type *T = LI.getType();
   if (!T->isAggregateType())
     return nullptr;
 
+  auto Name = LI.getName();
   assert(LI.getAlignment() && "Alignment must be set at this point");
 
   if (auto *ST = dyn_cast<StructType>(T)) {
     // If the struct only have one element, we unpack.
-    unsigned Count = ST->getNumElements();
-    if (Count == 1) {
+    auto NumElements = ST->getNumElements();
+    if (NumElements == 1) {
       LoadInst *NewLoad = combineLoadToNewType(IC, LI, ST->getTypeAtIndex(0U),
                                                ".unpack");
       return IC.ReplaceInstUsesWith(LI, IC.Builder->CreateInsertValue(
-        UndefValue::get(T), NewLoad, 0, LI.getName()));
+        UndefValue::get(T), NewLoad, 0, Name));
     }
 
     // We don't want to break loads with padding here as we'd loose
     // the knowledge that padding exists for the rest of the pipeline.
     const DataLayout &DL = IC.getDataLayout();
     auto *SL = DL.getStructLayout(ST);
     if (SL->hasPadding())
       return nullptr;
 
-    auto Name = LI.getName();
     SmallString<16> LoadName = Name;
     LoadName += ".unpack";
     SmallString<16> EltName = Name;
     EltName += ".elt";
+
     auto *Addr = LI.getPointerOperand();
-    Value *V = UndefValue::get(T);
-    auto *IdxType = Type::getInt32Ty(ST->getContext());
+    auto *IdxType = Type::getInt32Ty(T->getContext());
     auto *Zero = ConstantInt::get(IdxType, 0);
-    for (unsigned i = 0; i < Count; i++) {
+
+    Value *V = UndefValue::get(T);
+    for (unsigned i = 0; i < NumElements; i++) {
       Value *Indices[2] = {
         Zero,
         ConstantInt::get(IdxType, i),
       };
       auto *Ptr = IC.Builder->CreateInBoundsGEP(ST, Addr, makeArrayRef(Indices), EltName);
       auto *L = IC.Builder->CreateLoad(ST->getTypeAtIndex(i), Ptr, LoadName);
       V = IC.Builder->CreateInsertValue(V, L, i);
     }
 
     V->setName(Name);
     return IC.ReplaceInstUsesWith(LI, V);
   }
 
   if (auto *AT = dyn_cast<ArrayType>(T)) {
-    // If the array only have one element, we unpack.
-    if (AT->getNumElements() == 1) {
-      LoadInst *NewLoad = combineLoadToNewType(IC, LI, AT->getElementType(),
-                                               ".unpack");
+    auto *ET = AT->getElementType();
+    auto NumElements = AT->getNumElements();
+    if (NumElements == 1) {
+      LoadInst *NewLoad = combineLoadToNewType(IC, LI, ET, ".unpack");
       return IC.ReplaceInstUsesWith(LI, IC.Builder->CreateInsertValue(
-        UndefValue::get(T), NewLoad, 0, LI.getName()));
+        UndefValue::get(T), NewLoad, 0, Name));
+    }
+
+    SmallString<16> LoadName = Name;
+    LoadName += ".unpack";
+    SmallString<16> EltName = Name;
+    EltName += ".elt";
+
+    auto *Addr = LI.getPointerOperand();
+    auto *IdxType = Type::getInt32Ty(T->getContext());
+    auto *Zero = ConstantInt::get(IdxType, 0);
+
+    Value *V = UndefValue::get(T);
+    for (unsigned i = 0; i < NumElements; i++) {
+      Value *Indices[2] = {
+        Zero,
+        ConstantInt::get(IdxType, i),
+      };
+      auto *Ptr = IC.Builder->CreateInBoundsGEP(AT, Addr, makeArrayRef(Indices), EltName);
+      auto *L = IC.Builder->CreateLoad(ET, Ptr, LoadName);
+      V = IC.Builder->CreateInsertValue(V, L, i);
     }
+
+    V->setName(Name);
+    return IC.ReplaceInstUsesWith(LI, V);
   }
 
   return nullptr;
 }
 
 // If we can determine that all possible objects pointed to by the provided
 // pointer value are, not only dereferenceable, but also definitively less than
 // or equal to the provided maximum size, then return true. Otherwise, return
 // false (constant global values and allocas fall into this category).
 //
 // FIXME: This should probably live in ValueTracking (or similar).
 static bool isObjectSizeLessThanOrEq(Value *V, uint64_t MaxSize,
                                      const DataLayout &DL) {
   SmallPtrSet<Value *, 4> Visited;
   SmallVector<Value *, 4> Worklist(1, V);
 
   do {
     Value *P = Worklist.pop_back_val();
     P = P->stripPointerCasts();
 
     if (!Visited.insert(P).second)
       continue;
 
     if (SelectInst *SI = dyn_cast<SelectInst>(P)) {

test/Transforms/InstCombine/unpack-fca.ll

 ; CHECK-NEXT: store i8* null, i8** [[GEP1]], align 8
 ; CHECK-NEXT: [[GEP2:%[a-z0-9\.]+]] = getelementptr inbounds %B, %B* %b.ptr, i64 0, i32 1
 ; CHECK-NEXT: store i64 42, i64* [[GEP2]], align 8
 ; CHECK-NEXT: ret %B { i8* null, i64 42 }
   store %B { i8* null, i64 42 }, %B* %b.ptr, align 8
   %1 = load %B, %B* %b.ptr, align 8
   ret %B %1
 }
+
+define [2 x %B] @loadArrayOfB([2 x %B]* %ab.ptr) {
+; CHECK-LABEL: loadArrayOfB
+; CHECK-NEXT: [[GEP1:%[a-z0-9\.]+]] = getelementptr inbounds [2 x %B], [2 x %B]* %ab.ptr, i64 0, i64 0, i32 0
+; CHECK-NEXT: [[LOAD1:%[a-z0-9\.]+]] = load i8*, i8** [[GEP1]], align 8
+; CHECK-NEXT: [[IV1:%[a-z0-9\.]+]] = insertvalue %B undef, i8* [[LOAD1]], 0
+; CHECK-NEXT: [[GEP2:%[a-z0-9\.]+]] = getelementptr inbounds [2 x %B], [2 x %B]* %ab.ptr, i64 0, i64 0, i32 1
+; CHECK-NEXT: [[LOAD2:%[a-z0-9\.]+]] = load i64, i64* [[GEP2]], align 8
+; CHECK-NEXT: [[IV2:%[a-z0-9\.]+]] = insertvalue %B [[IV1]], i64 [[LOAD2]], 1
+; CHECK-NEXT: [[IV3:%[a-z0-9\.]+]] = insertvalue [2 x %B] undef, %B [[IV2]], 0
+; CHECK-NEXT: [[GEP3:%[a-z0-9\.]+]] = getelementptr inbounds [2 x %B], [2 x %B]* %ab.ptr, i64 0, i64 1, i32 0
+; CHECK-NEXT: [[LOAD3:%[a-z0-9\.]+]] = load i8*, i8** [[GEP3]], align 8
+; CHECK-NEXT: [[IV4:%[a-z0-9\.]+]] = insertvalue %B undef, i8* [[LOAD3]], 0
+; CHECK-NEXT: [[GEP4:%[a-z0-9\.]+]] = getelementptr inbounds [2 x %B], [2 x %B]* %ab.ptr, i64 0, i64 1, i32 1
+; CHECK-NEXT: [[LOAD4:%[a-z0-9\.]+]] = load i64, i64* [[GEP4]], align 8
+; CHECK-NEXT: [[IV5:%[a-z0-9\.]+]] = insertvalue %B [[IV4]], i64 [[LOAD4]], 1
+; CHECK-NEXT: [[IV6:%[a-z0-9\.]+]] = insertvalue [2 x %B] [[IV3]], %B [[IV5]], 1
+; CHECK-NEXT: ret [2 x %B] [[IV6]]
+  %1 = load [2 x %B], [2 x %B]* %ab.ptr, align 8
+  ret [2 x %B] %1
+}