[PATCH] D86050: [SCEV] Refactor isHighCostExpansionHelper

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2056-2064
     if (OrigPhiRef->getType()->isPointerTy() != Phi->getType()->isPointerTy())
       continue;
 
     if (BasicBlock *LatchBlock = L->getLoopLatch()) {
       Instruction *OrigInc = dyn_cast<Instruction>(
           OrigPhiRef->getIncomingValueForBlock(LatchBlock));
       Instruction *IsomorphicInc =
----------------
samparker wrote:
> lebedev.ri wrote:
> > I guess i'm starting getting confused again and thus stalling, but.
> > 
> > >>! In D86050#2232843, @samparker wrote:
> > >> Please can you explain why the calculated costs changed for those X86 tests?
> > > These were introduced once the Add and Mul expressions were costed in the same manner as when we're calculating for an AddRec.
> > 
> > Right. But why was that done in the first place? Was that intentional?
> > That doesn't look correct to me.
> > 
> > Plain `Add` and `Mul` are *not* identical to `AddRec`.
> > 
> > > I'll remove the change for this patch and add it back in a follow-up.
> It was intentional... Why wouldn't we want to check for constant zero for plain Adds and constant one for plain Muls?
> Why wouldn't we want to check for constant zero for plain Adds and constant one for plain Muls?
Because they shouldn't be there, they should be canonicalized away during SCEV creation.

But my main question is, why did that result in *higher* cost, as it can be seen from the blocked transforms?


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

https://reviews.llvm.org/D86050