[PATCH] D97863: [LSR] adjust isHighCostExpansion function for Mul case

[Danila Malyutin via Phabricator via llvm-commits]

danilaml added a comment.

This is mostly to get the comments on the original function and it's intent. Before this change the test would bail out on a first Instruction::Mul user which doesn't make much sense from the context. I.e. if there were two Mul users, one satisfying the ` SE.getSCEV(UI) == Mul` condition, then whether the increment would be considered high cost would depend on the order of the Users in the users() list.
Also, it would return `true` (meaning high cost) if the existing instruction already computes given SCEV, which seems to be the opposite of the stated intent unless I'm misunderstanding something.

Another approach that was suggested is to replace this function with the SCEVExpander::isHighCostExpansion, however it doesn't seem to be trivial because 1) this is called at earlier stage when the IV chains are collected so I don't think there is any info like the insertion point and the like and creating the SCEVExpander instance solely to call the isHighCostExpansion feels weird 2) Those functions have different defaults (LSR one is high-cost-by-default) and different checks (i.e. this does some Phi checks).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97863