[PATCH] D27695: Add Instruction number to LSR cost model (PR23384)

[Evgeny Stupachenko via Phabricator via llvm-commits]

evstupac added a comment.

Hi Quentin,

Thanks for taking a look.

> Like Wei, I'd like #3 to be a separate patch.
>  Indeed, #1 and #2 should be NFC as long as the targets do not change the cost model.

Ok. I'll split the patch.
However #3 without #2 has almost no effect. So the following order seems the best #1, #3, #2.

Thanks,
Evgeny



================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:1874
+                    C2.NumIVMuls, C2.NumBaseAdds,
+                    C2.ScaleCost, C2.ImmCost, C2.SetupCost);
+}
----------------
qcolombet wrote:
> Do you have data to support that heuristic?
> Like Wei said, I suspect this may lead to pretty bad side effect where we will increase the register pressure by a lot to save a few instruction.
> So before we switch the default, I want supportive evidence that this is general goodness.
> 
> For the record, we discuss with Wei this cost model issue and I still have on my todolist a better register pressure estimation for the loop. E.g., we can ignore NumRegs as long as it is below the regpressure.
>Do you have data to support that heuristic?
Yes on spec2000:

177.mesa on -O2 +3%
256.bzip2 on -Ofast -flto +1.5%

There are gains on EEMBC tests. I don't see any significant (>2% regressions). And I'm waiting for data from Wei.
However I agree that for better testing we can introduce an option like -lsr-count-insns.

> E.g., we can ignore NumRegs as long as it is below the regpressure.
That is roughly estimated in my patch. When TTI.getNumberOfRegisters(false) - 1 is exceeded we start to increment instruction counter (1 insn per 1 new register). In most cases that is enough to take a solution with low register pressure.




Repository:
  rL LLVM

https://reviews.llvm.org/D27695