[PATCH] D12066: Introduce the ScopExpander as a SCEVExpander replacement

[Tobias Grosser via llvm-commits]

jdoerfert wrote:
>grosser wrote:
>> Do we happen to have a test case covering this use? Meaning a loop-variant srem node with data-dependences accross basic-blocks that would crash without this addition?
> No and I do not think we would crash here anyway. I think only the SDiv/SRem is a parameter of the SCoP case can crash. However, I changed all Expander locations for two reasons:
>    1) We have a clear interface now that hides how we actually expand code.
>    2) We have one location where we need to handle new features later on.

OK, that's fine.

> ================
> Comment at: lib/Support/ScopHelper.cpp:270
> @@ +269,3 @@
> +    if (!S.getRegion().contains(I))
> +      E = visit(E);
> +    return Expander.expandCodeFor(E, Ty, I);
> ----------------
> grosser wrote:
>> Why don't we use the ScopExpander in the region? Even inside the region we
>> may work at places where I does not dominate all SCEVUnknowns, no?
> Like I said above. I don't think so. Even if that might be the case, the current handling might not work then. For now I see 3 cases we expands SCEVs:
>    1) IslExprBuilder: Here we create (mainly) loop bounds etc. (inside the region) and the problem should not occure since we generate the full expression (except the parameters) from scratch.
>    2) BlockGenerator: Here we synthezise values during code generation (inside the region) but again, everything except the parameters are generated from scratch (afaik).
>    3) IslNodeBuilder: Here we generate code for the parameters (in front of the region) and we can crash. However, only if the parameters contains instructions in the region.
>
> If the above observation is correct the code should be too. If not or we extend/change Polly at some point the new structure should allows us to modify the behavior of the SCEV expansion pretty easily.

A test case for this is test/Isl/CodeGen/srem-in-other-bb.ll.

It works today as we IndependentBlocks just moves the full operand tree into the
right location. If we disable independent blocks entirely, scalar dependences
are introduced and the values are propagated via memory. So it seems the
way we are currently handling such cases is save, but could be improved in the future
by possibly dropping independent blocks entirely, not model the memory dependences and
then fully expand these extended SCEV expressions during code generation.

However, I believe this is out of scope for this patch.

Feel free to commit if this patch passes LNT for you.

Best,
Tobias