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 ↗

(On Diff #46581)

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

531 ↗

(On Diff #46581)

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

582 ↗

(On Diff #46581)

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 ↗

(On Diff #46581)

Is that a

526 ↗

(On Diff #46581)

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

582 ↗

(On Diff #46581)

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 ↗

(On Diff #49390)

Arrays can have more than 2**32 elements.

602 ↗

(On Diff #49390)

You should use uint64_t here.

[deadalnix]

Use 64 bits indices

[majnemer]

LGTM

llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

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::getInt64Ty(T->getContext());
+    auto *Zero = ConstantInt::get(IdxType, 0);
+
+    Value *V = UndefValue::get(T);
+    uint64_t Offset = 0;
+    for (uint64_t 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

llvm/trunk/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