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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:55
     case Instruction::GetElementPtr:
       llvm_unreachable("Use getGEPCost for GEP operations!");
 
----------------
junbuml wrote:
> 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)  
> 
> 
> 
Yes. Then implement this new function so that it calls the instruction-independent version to handle the non-user-based logic.


================
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
----------------
junbuml wrote:
> 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. 
Why a separate patch? This patch is not overly complicated, and if there's a separate patch we'll have a regression in between.

If you want to separate the patches, we should have them both before either is committed. But it sounds like a couple lines of code and a few lines of code for some tests.



https://reviews.llvm.org/D38085