Differential D31194
[InstSimplify] Try to Constant Fold the Instruction before simplification Authored by joey on Mar 21 2017, 10:16 AM.
Details
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. Noticed a change in IR after r276727 (e37fa0be6d80d863647db12f0d8e8c7d62bf6f3d).
Diff Detail Event TimelineComment Actions 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. Comment Actions 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? Comment Actions 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. Comment Actions 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.
Comment Actions Looks like ConstantFoldCompareInstOperands in lib/Analysis/ConstantFolding.cpp can't handle icmp eq null, (inttoptr x) it can only handle null on the RHS. Comment Actions I've submitted a separate patch to fix the deficiency in ConstantFold which should recover completeness.ll Comment Actions 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?
| ||||||||||||||||||||||||
Do you need to llvm:: qualify this call?