[PATCH] D128302: [AArch64][CostModel] Detects that ExtractElement at index is not free in AArch64 when result is used as integer.

Thu Jun 30 15:05:40 PDT 2022

mingmingl added inline comments.

================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1868
+        Instruction *UserI = dyn_cast<Instruction>(U.getUser());
+        // FIXME: Shall all users be of type Instruction?
+        if (!UserI)
----------------
mingmingl wrote:
> RKSimon wrote:
> > mingmingl wrote:
> > > nikic wrote:
> > > > Yes, all users of an instruction are instructions, it's safe to use `cast` here.
> > > thanks for taking a look! I looked at a few existing cpp and they also assume so.
> > > 
> > > Changing this to an assert then.
> > Scanning the uses like this isn't going to be cheap - and getCost calls can be called a LOT.
> > 
> > Why did you decide this was necessary? Do existing tests change without it? You don't appear to have added any test coverage for these cases.
> Scanning cost is a good point; one mitigation (in the patch) is that, there is a `break` out of the loop once an integer use case if ound.
> 
> May I measure the compile time increase with https://github.com/nikic/llvm-compile-time-tracker and report back? (leaving this open)
Compared between HEAD~1 and this patch gives https://llvm-compile-time-tracker.com/compare.php?from=1e556f459b44dd0ca4073e932f66ecb6f40fe31a&to=63035f714c94d31698b5b4ab79b9912814c29345&stat=instructions

Across different configs and benchmarks, the compile-time increases or decreases within +/- 0.1%. I wonder if this means it's fine in terms of the compile-time cost?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128302