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?

588

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.

588

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

597

Arrays can have more than 2**32 elements.

602

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.
     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, 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.
for (unsigned i = 0; i < NumElements; i++) {
       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));
     }
+
+    const DataLayout &DL = IC.getDataLayout();
+    auto EltSize = DL.getTypeAllocSize(ET);
+    auto Align = LI.getAlignment();
+    if (!Align)
+      Align = DL.getABITypeAlignment(T);
+
+    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);
+    uint64_t Offset = 0;
+    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->CreateAlignedLoad(Ptr, MinAlign(Align, Offset),
+                                              LoadName);
+      V = IC.Builder->CreateInsertValue(V, L, i);
+      Offset += EltSize;
+    }
+
+    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

test/Transforms/InstCombine/unpack-fca.ll

 ; 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
+}
+
 %struct.S = type <{ i8, %struct.T }>
 %struct.T = type { i32, i32 }
 
 ; Make sure that we do not increase alignment of packed struct element
 define i32 @packed_alignment(%struct.S* dereferenceable(9) %s) {
 ; CHECK-LABEL: packed_alignment
 ; CHECK-NEXT: %tv.elt1 = getelementptr inbounds %struct.S, %struct.S* %s, i64 0, i32 1, i32 1
 ; CHECK-NEXT: %tv.unpack2 = load i32, i32* %tv.elt1, align 1