[InstSimplify] Try to Constant Fold the Instruction before simplification
ClosedPublic

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

Details

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.Mar 31 2017, 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.Apr 21 2017, 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.

joey updated this revision to Diff 101034.Thu, Jun 1, 8:47 AM

Constant fold inside SimplifyGEPInst instead.

dberlin added inline comments.Thu, Jun 1, 8:56 AM
test/Transforms/NewGVN/completeness.ll
403

Did you change this manually?
We normally use update_test_checks on these tests, and i've never seen it do that.
(also, the update would mean we lost optimization, since it should stay false)

dberlin added inline comments.Thu, Jun 1, 9:24 AM
test/Transforms/NewGVN/completeness.ll
403

Just tested it.
It's definitely lost optimization.
We are not longer able to simplify that br i1 to false, because we don't simplify the phi to false with this patch,.

Looks like ConstantFoldCompareInstOperands in lib/Analysis/ConstantFolding.cpp can't handle icmp eq null, (inttoptr x) it can only handle null on the RHS.

I've submitted a separate patch to fix the deficiency in ConstantFold which should recover completeness.ll

joey updated this revision to Diff 101179.Fri, Jun 2, 3:10 AM

Thanks for that Craig, I had planned on looking at that today so thanks for doing it!

I tested with D33801 applied, and it fixes the completeness.ll test.

Once Craig's review is finished, is this good to commit?

This revision is now accepted and ready to land.Sun, Jun 4, 3:35 PM
majnemer added inline comments.Sun, Jun 4, 9:34 PM
lib/Analysis/InstructionSimplify.cpp
3905

Do you need to llvm:: qualify this call?

joey closed this revision.Tue, Jun 6, 3:26 AM

Committed this as r304784.

I removed the 'llvm::' as David spotted.