[PATCH] D28307: Add Instruction number to LSR cost model (PR23384) part 1 of 3

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet requested changes to this revision.
qcolombet added a comment.
This revision now requires changes to proceed.

Hi,

I'd like to see `opt` tests as well and one piece of the code seems dead to me unless I am missing something. See my inline comment.

Also a related question, what is your plan to move with that heuristic?

Generally speaking, I think the number of instructions is a good heuristic only for code size and it is not even a given. Indeed, for performance we care about critical path, IPL, this kind of thing. What am I saying is having more instructions is not necessarily a bad thing. The other thing is I believe we can end up with cases with less instructions, but more registers and potentially more spill. This is possible to happen because the heuristic that account for "spill" only trigger when the NumRegs gets high enough. However, NumRegs may be low enough and spill may already happen. See my comment below as well.

Cheers,
-Quentin



================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:1195
+  unsigned TTIRegNum = TTI.getNumberOfRegisters(false) - 1;
+  if (NumRegs > TTIRegNum) {
+    // Cost already exceeded TTIRegNum, then only newly added register can add
----------------
With this kind of check we are (sort of) sure we are going to spill to materialize this formulae, but that does not mean the other don't.
Bare in mind that NumRegs is only what's use in the formulae, not what is live.

Your comment is right, just wanted to make sure we are on the same page. 


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:1198
+    // new instructions.
+    if (NumRegs > TTIRegNum)
+      Insns += (NumRegs - PrevNumRegs);
----------------
I'm confused, this was tested in the condition just before, i.e., it's always going to be true and the else statement is always going to be false.

Am I missing something?


================
Comment at: test/Transforms/LoopStrengthReduce/X86/lsr-insns-1.ll:1
+; RUN: llc < %s -O2 -march=x86-64 -lsr-insns-cost -asm-verbose=0 | FileCheck %s
+
----------------
I would like to see opt -loop-reduce checks as well.


================
Comment at: test/Transforms/LoopStrengthReduce/X86/lsr-insns-2.ll:1
+; RUN: llc < %s -O2 -march=x86-64 -lsr-insns-cost -asm-verbose=0 | FileCheck %s
+
----------------
Ditto.


Repository:
  rL LLVM

https://reviews.llvm.org/D28307