[PATCH] D31194: [InstSimplify] Try to Constant Fold the Instruction before simplification

[Joey Gouly via Phabricator via llvm-commits]

joey added a comment.

In https://reviews.llvm.org/D31194#713113, @majnemer wrote:

> 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?


Repository:
  rL LLVM

https://reviews.llvm.org/D31194