[InstSimplify] Try to Constant Fold the Instruction before simplification
Needs ReviewPublic

Authored by joey on Mar 21 2017, 10:16 AM.

Details

Reviewers
majnemer
Summary

Call ConstantFoldInstruction before trying any other simplification.

test/Analysis/ConstantFolding/gep-constanfolding-error.ll has a regex because there is a difference in the integer types after old/new GVN.
test/Transforms/InstSimplify/vector_gep.ll, I am unsure why the indices for the vector are turned into i64.

Noticed a change in IR after r276727 (e37fa0be6d80d863647db12f0d8e8c7d62bf6f3d).

Diff Detail

joey created this revision.Mar 21 2017, 10:16 AM
svenvh added a subscriber: svenvh.Mar 21 2017, 11:15 AM

Do you have insight into how this differs from what we already do? My understanding is that each of the simplify routines immediately try to constant fold.

joey added a comment.Fri, Mar 31, 8:13 AM

Do you have insight into how this differs from what we already do? My understanding is that each of the simplify routines immediately try to constant fold.

I dug into this more, and found out it's actually the GEP that is not being constant folded.

Before my patch:

<badref> = getelementptr inbounds %struct.__block_literal_generic, %struct.__block_literal_generic addrspace(4)* addrspacecast (%struct.__block_literal_generic addrspace(1)* bitcast (%__aaa_struct addrspace(1)* @__aaa_struct_ptr to %struct.__block_literal_generic addrspace(1)*) to %struct.__block_literal_generic addrspace(4)*), i64 0, i32 3

Is InstructionSimplified to:

i8* addrspace(4)* getelementptr (%struct.__block_literal_generic, %struct.__block_literal_generic addrspace(4)* addrspacecast (%struct.__block_literal_generic addrspace(1)* bitcast (%__aaa_struct addrspace(1)* @__aaa_struct_ptr to   %struct.__block_literal_generic addrspace(1)*) to %struct.__block_literal_generic addrspace(4)*), i64 0, i32 3)

After my patch:

<badref> = getelementptr inbounds %struct.__block_literal_generic, %struct.__block_literal_generic addrspace(4)* addrspacecast (%struct.__block_literal_generic addrspace(1)* bitcast (%__aaa_struct addrspace(1)* @__aaa_struct_ptr to %struct.__block_literal_generic addrspace(1)*) to %struct.__block_literal_generic addrspace(4)*), i64 0, i32 3

Is InstructionSimplified to:

i8* addrspace(4)* getelementptr inbounds (%__aaa_struct, %__aaa_struct addrspace(4)* addrspacecast (%__aaa_struct addrspace(1)* @__aaa_struct_ptr to %__aaa_struct addrspace(4)*), i64 0, i32 0, i32 3)

If you look at SimplifyGEPInst, it doesn't actually call any constant folding, like the other Simplify*Inst functions do.

I could possibly add some code to run a constant folder here:

// Check to see if this is constant foldable.
for (unsigned i = 0, e = Ops.size(); i != e; ++i)
  if (!isa<Constant>(Ops[i]))
    return nullptr;

return ConstantExpr::getGetElementPtr(SrcTy, cast<Constant>(Ops[0]),
                                      Ops.slice(1));

This leads to the following patch, which has smaller scope:

diff --git a/lib/Analysis/InstructionSimplify.cpp b/lib/Analysis/InstructionSimplify.cpp
index 08afafa..4eb0759 100644
--- a/lib/Analysis/InstructionSimplify.cpp
+++ b/lib/Analysis/InstructionSimplify.cpp
@@ -3913,8 +3913,11 @@ static Value *SimplifyGEPInst(Type *SrcTy, ArrayRef<Value *> Ops,
     if (!isa<Constant>(Ops[i]))
       return nullptr;
 
-  return ConstantExpr::getGetElementPtr(SrcTy, cast<Constant>(Ops[0]),
-                                        Ops.slice(1));
+  auto *CE = ConstantExpr::getGetElementPtr(SrcTy, cast<Constant>(Ops[0]),
+                                            Ops.slice(1));
+  if (auto *CEFolded = llvm::ConstantFoldConstant(CE, Q.DL))
+    return CEFolded;
+  return CE;
 }
 
 Value *llvm::SimplifyGEPInst(Type *SrcTy, ArrayRef<Value *> Ops,

It also requires some test changes, but I didn't include them here.

Do you think I should go with this new patch?

I wonder if you should just call ConstantFoldInstOperands where we currently call ConstantExpr::getElementPtr? I think that will try to constant fold it before falling back to ConstantExpr::getElementPtr.

joey added a comment.Fri, Apr 21, 2:56 AM

I wonder if you should just call ConstantFoldInstOperands where we currently call ConstantExpr::getElementPtr? I think that will try to constant fold it before falling back to ConstantExpr::getElementPtr.

Unless I'm misunderstanding, 'ConstantFoldInstOperands' requires an Instruction as an argument and at the point inside 'SimplifyGEPInst', I don't have an Instruction just the operands.
I could create an Instruction and pass it to 'ConstantFoldInstOperands', but that seems just the same as what I suggested.