[PATCH] D138937: [AMDGPU] Update InstrCost calculation

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPerfHintAnalysis.cpp:228
+      // skip bitcast for cost calculation
+      if (I.isCast())
+        continue;
----------------
yassingh wrote:
> arsenm wrote:
> > I'm pretty sure this skips a lot more casts.
> > 
> > This also looks backwards. Opaque pointers avoid a lot of bitcasts, not introduce them
> Updated bitcast check. 
> //"This also looks backwards. Opaque pointers avoid a lot of bitcasts, not introduce them"//
> Not sure how else to add this check. Should we entirely avoid this check and update test to use opaque pointers?
I can see this help get the same results for typed and opaque pointers, but in the direction that hurts the case you're looking at?

All tests need to be updated to use opaque pointers. Typed pointers are off by default and are only still around because so many tests need to be updated


================
Comment at: llvm/test/CodeGen/AMDGPU/perfhint-instr-cost.ll:1
+; RUN: llc -march=amdgcn -debug-only=amdgpu-perf-hint < %s 2>&1 | FileCheck %s
+define i32 @perfHintInstrCost(ptr addrspace(4) %p1, ptr addrspace(4) %p2, ptr addrspace(4) %p3) #0 {
----------------
If you're going to directly check the debug output, you need REQUIRES: asserts


================
Comment at: llvm/test/CodeGen/AMDGPU/perfhint.ll:2
 ; RUN: llc -march=amdgcn < %s | FileCheck -check-prefix=GCN %s
+; RUN: llc -march=amdgcn -opaque-pointers=1 < %s | FileCheck -check-prefix=GCN %s
 
----------------
This is the wrong way to switch this. You need to textually replace all the pointers in the test. You can run https://gist.github.com/nikic/98357b71fd67756b0f064c9517b62a34, although that should be in a separate change from any functional patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138937