[PATCH] D38085: Use the basic cost if a GEP is not used as addressing mode

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added inline comments.


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:55
     case Instruction::GetElementPtr:
       llvm_unreachable("Use getGEPCost for GEP operations!");
 
----------------
hfinkel wrote:
> This needs to be updated, or...
> 
> At a high level, I'm not sure what we want to completely remove the version of the function that can be used without an existing function. Instead, we should add an overload, and then fall back to the existing code where relevant.
> 
I'm not perfectly clear about this. 
Do you think we should keep the existing getGEPCost as it is : 
  int getGEPCost(Type *PointeeType, const Value *Ptr, ArrayRef<const Value *> Operands),
and add the new one :
  int getGEPCost(const GEPOperator *GEP, ArrayRef<const Value *> Operands)  





================
Comment at: test/Analysis/CostModel/X86/vector_gep.ll:11
   %vector = shufflevector <4 x i64> %temp, <4 x i64> undef, <4 x i32> zeroinitializer
-;CHECK: cost of 0 for instruction: {{.*}} getelementptr inbounds %struct.S
+;CHECK: cost of 1 for instruction: {{.*}} getelementptr inbounds %struct.S
   %B = getelementptr inbounds %struct.S, <4 x %struct.S*> %s, <4 x i32> zeroinitializer, <4 x i32> zeroinitializer
----------------
efriedma wrote:
> junbuml wrote:
> > With this patch, %B is changed to a non-free because it's used in %arrayidx (non-memory operation). It might be possible to continue checking users of the non-memory operation users, but doing this completely must be expensive to be done in getGEPCost.  It might be possible to add some simple exceptions, but in this patch I didn't add such checks.
> Yes, it should be fine to avoid folding together GEPs in getUserCost().  (Arguably, you might want to, but it could get complicated, so okay to skip that for now.)
> 
> That said, there's something going wrong here. "gep %x, 0, 0" is free because it's just a type conversion.  By the same reasoning, "gep %s, zeroinitializer, zeroinitializer" should also be free.
We might be able to handle zeroinlitializer in hasAllZeroIndices() for ArrayType.  Then, I think it should be a separate patch. 


https://reviews.llvm.org/D38085