[PATCH] D86050: [SCEV] Refactor isHighCostExpansionHelper

[Sam Parker via Phabricator via llvm-commits]

samparker 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 =
----------------
lebedev.ri wrote:
> 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?
I don't know, it didn't make sense to me. Anyway, it's not in the patch anymore.


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

https://reviews.llvm.org/D86050