[PATCH] D149741: [InlineCost][TargetTransformInfo][AMDGPU] Consider cost of alloca instructions in the caller (2/2)

[Juan Manuel Martinez Caamaño via Phabricator via llvm-commits]

jmmartinez added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp:1257
+      AllocaSize;
+  // awkwardly, this gets multiplied by the vector and single-bb bonuses
+  return ThresholdBonus;
----------------
Pierre-vh wrote:
> Is it an issue? Can you elaborate a bit more?
After adding the inline threshold bonus that comes from adjustInliningThreshold, the threshold gets multiplied by threshold-multiplier, the single-bb bonus, and the vector bonus.

The cost assigned to each alloca assumes that 

   Cost_Alloca_0 + ... + Cost_Alloca_N == ( ArgAllocaCost * threshold-multiplier )

But it doesn't take into acount the single-bb bonus and the vector bonus.

This may give an inlining advantage to functions with a single-bb or with vector instructions that was not there before.

The single-bb could be easily fixed tough.

I feel that this patch tries to fit the problem to the solution rather than the opposite :S 


================
Comment at: llvm/test/Transforms/Inline/AMDGPU/amdgpu-inline-alloca-argument-cost.ll:8
 ; Verify we are properly adding cost of the -amdgpu-inline-arg-alloca-cost to the threshold.
+; Awkwardly, the adjusted threshold is multiplied by the single-basic-block bonus in .
 
----------------
Fix comment.


================
Comment at: llvm/test/Transforms/Inline/AMDGPU/amdgpu-inline-alloca-argument-cost.ll:95
+declare void @external(ptr addrspace(5) %p)
\ No newline at end of file

----------------
Pierre-vh wrote:
> newline at end of file :) 
> (I also had this a lot until I found a setting in my IDE to automatically insert it on save, if you use VSCode it's easy)
Thanks for the tip!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149741/new/

https://reviews.llvm.org/D149741