[PATCH] D14296: [LV][LAA] Add a layer over SCEV to apply run-time checked knowledge on SCEV expressions

[silviu.baranga@arm.com via llvm-commits]

sbaranga added a comment.

Hi James,

In http://reviews.llvm.org/D14296#286968, @jmolloy wrote:

> Hi,
>
> So most of this patch is just plumbing and renaming - the guts is the addition of SCEVPredicateLayer.


Correct.

> Bikeshedding: I'm not too happy with the name. SCEV is the prefix used for the internal node types of ScalarEvolution. Something like PredicatedScalarEvolution might sound better?


I don't have a strong preference here.

> I'm a bit worried about how easy it is to subvert the predicated mechanism. For example:

> 

>   PSE.getSCEV(); // Gets a scev under a predicate

>   PSE.getSE()->getSCEV(); // Gets a scev not under a predicate


Yes, but the two have different meanings (they are not equal when the predicate evaluates to false), so you should be able to do both things?

> And there are places in LoopVectorize that sem to use "PSE.getSE()" quite a bit... are we sure they aren't subverting the predicates? If they do, is that a correctness or a performance issue?


It would only be a correctness problem if you would expect some expression to have some form and only the predicated version would have that problem. 
Otherwise SE.getSCEV() and PredSE.getSCEV() would produce expressions evaluating to the same values (as long as we pass the runtime checks).
I've audited all the uses in LV and I believe there wouldn't be any correctness problem. There might be issues in other passes (LLE in particular), which I plan to address in a future commit.

> If it's just a performance thing, it might be nice to make use of ScalarEvolution even easier by defining an operator->/operator* so you can do

> 

>   PSE->getEqualPredicate();

>   PSE.getSCEV();


That's an interesting idea. You could still do PSE->getSCEV() though?

-Silviu


http://reviews.llvm.org/D14296