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

[Mingming Liu via Phabricator via llvm-commits]

mingmingl marked 2 inline comments as done.
mingmingl added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:92
+    Cost += thisT()->getVectorInstrCost(Instruction::ExtractElement, VTy, 0,
+                                        nullptr);
 
----------------
RKSimon wrote:
> any chance we can add a default (nullptr) arg value and avoid all the nullptr additions?
This is a good question. 

If we want to have a default (nullptr) arg, each derived class (AArch64TargetTransformInfo, X86TargetTransformInfo, etc) needs to override the default pointer argument, since derived classes won't inherit the default argument of base class's virtual method [1] and it's suggested not to "change the default parameters of overridden inherited functions" [2].

I don't feel strong about this, but not adding a default following the existing style of `Index` -> `Index` has a default value of -1 in class `TargetTransformInfo`, but not in class `BasicTTIImpl` or derived classes.

May I know your thoughts about this? (leaving this comment open)

[1] https://stackoverflow.com/questions/3533589/can-virtual-functions-have-default-parameters
[2] http://www.gotw.ca/gotw/005.htm


================
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)
----------------
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)


================
Comment at: llvm/test/Analysis/CostModel/AArch64/kryo.ll:43
+    ret i64 %val
+}
----------------
RKSimon wrote:
> pre-commit the test with trunk codegen and rebase the patch to show the change in cost reporting
will do in another patch (got to reply first for compile-time question around scanning `uses()`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128302