[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:
> > 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.
> I don't know, it didn't make sense to me.

Honestly, recalling how many of these costmodelling patches were there recently,
i'm now somewhat worried that i didn't pay attention to them..


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

https://reviews.llvm.org/D86050