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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


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


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1860
+        [Val](const Instruction *Inst) -> bool {
+      if (Inst == nullptr || (!isa<ExtractElementInst>(Inst)) ||
+          (!Val->getScalarType()->isIntegerTy()))
----------------
!isa_and_nonnull<ExtractElementInst>(Inst) ?


================
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:
> 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.


================
Comment at: llvm/test/Analysis/CostModel/AArch64/kryo.ll:37
+    ;
+    ; CHECK: cost of 2 {{.*}} extractelement <4 x i64> %vecreg, i32 0
+    %t0 = extractelement <4 x i64> %vecreg, i32 0
----------------
Add the other CHECKs for other extraction indices?


================
Comment at: llvm/test/Analysis/CostModel/AArch64/kryo.ll:43
+    ret i64 %val
+}
----------------
pre-commit the test with trunk codegen and rebase the patch to show the change in cost reporting


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128302