llvm dev - Sep 2016 - Problem with "[SimplifyCFG] Handle tail-sinking of more than 2 incoming branches"

Hello,

I’m getting some failures on our internal testing happening after this commit.

I dug a little bit into it and it ended up begin caused by the fact that this
optimization is sinking some instructions that shouldn’t be sunk

In particular we have some loads that need to have a constant address to be
selected, because they load from a “special address space”.

What the optimization is doing is extracting the “getelementptr” , making an
instruction out of it, sinking the load and using a PHI to select between the
address.
This breaks our selection of this types of loads.

We need a way to tell this code to not do that.

The solutions I kind of see for this is:

- Add some TTI hook that stops this sinking from happening for instructions that
don’t support that like in this case (maybe added in canSinkInstructions())

- Add some TTI hook that is a little bit coarser that stops the optimization to
run altogether. In particular this thing is creating “unstructured control-flow”
in some cases and this is not always good (and actually generally bad) on GPU
targets.

- Put the optimization in another separate pass. It seems to be doing stuff that
is a little bit more than just “Simplifying the CFG” actually. It is actually
making it more complex by splitting blocks around. I wonder if it would make
sense for it to live it in a separate pass that can be enabled or disabled at
the compiler pipeline level so that one that wants to opt out can just avoid to
add the pass to the pipeline.

Open for suggestions on what the best solution for this is.

Thanks,
Marcello

Hello Marcello,

We're currently planning to disable this optimization - at least
temporarily - for cases where we'll end up creating PHIs of pointers for
performance reasons (see PR30188).

Having said that - I don't believe this is something you can rely on for
correctness.
>From an IR semantics perspective, this is a completely legal transformation
that can be introduced by any optimization pass, for whatever reason. We
can't control that throughout the entire optimization stack with a TTI
hook. Two potential solutions I can see are to either (a) teach your
backend to handle it (by disambiguating these loads in a pre-ISel IR pass),
or (b) use some kind of type-system trick to make it impossible (say,
assign a different address space to each distinct pointer in the front end).

Thanks,
  Michael

On Fri, Sep 2, 2016 at 11:39 AM, Marcello Maggioni via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Hello,
>
> I’m getting some failures on our internal testing happening after this
> commit.
>
> I dug a little bit into it and it ended up begin caused by the fact that
> this optimization is sinking some instructions that shouldn’t be sunk
>
> In particular we have some loads that need to have a constant address to
> be selected, because they load from a “special address space”.
>
> What the optimization is doing is extracting the “getelementptr” , making
> an instruction out of it, sinking the load and using a PHI to select
> between the address.
> This breaks our selection of this types of loads.
>
> We need a way to tell this code to not do that.
>
> The solutions I kind of see for this is:
>
> - Add some TTI hook that stops this sinking from happening for
> instructions that don’t support that like in this case (maybe added in
> canSinkInstructions())
>
> - Add some TTI hook that is a little bit coarser that stops the
> optimization to run altogether. In particular this thing is creating
> “unstructured control-flow” in some cases and this is not always good (and
> actually generally bad) on GPU targets.
>
> - Put the optimization in another separate pass. It seems to be doing
> stuff that is a little bit more than just “Simplifying the CFG” actually.
> It is actually making it more complex by splitting blocks around. I wonder
> if it would make sense for it to live it in a separate pass that can be
> enabled or disabled at the compiler pipeline level so that one that wants
> to opt out can just avoid to add the pass to the pipeline.
>
> Open for suggestions on what the best solution for this is.
>
> Thanks,
> Marcello
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160902/43993c00/attachment.html>

Probably the issue is solvable in some Codegen prepare pass.

That said I still believe some kind of control on if we would like to implement
this or not could be useful.

Just a question. Why implementing it in SimplifyCFG and not as a separate pass
like JumpThreading or something like that? The transformation itself doesn’t
seem to fit much in SimplifyCFG.
> On 2 Sep 2016, at 13:35, Michael Kuperstein <mkuper at google.com>
wrote:
> 
> Hello Marcello,
> 
> We're currently planning to disable this optimization - at least
temporarily - for cases where we'll end up creating PHIs of pointers for
performance reasons (see PR30188).
> 
> Having said that - I don't believe this is something you can rely on
for correctness.
> From an IR semantics perspective, this is a completely legal transformation
that can be introduced by any optimization pass, for whatever reason. We
can't control that throughout the entire optimization stack with a TTI hook.
Two potential solutions I can see are to either (a) teach your backend to handle
it (by disambiguating these loads in a pre-ISel IR pass), or (b) use some kind
of type-system trick to make it impossible (say, assign a different address
space to each distinct pointer in the front end).
> 
> Thanks,
>   Michael
> 
> On Fri, Sep 2, 2016 at 11:39 AM, Marcello Maggioni via llvm-dev
<llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>>
wrote:
> Hello,
> 
> I’m getting some failures on our internal testing happening after this
commit.
> 
> I dug a little bit into it and it ended up begin caused by the fact that
this optimization is sinking some instructions that shouldn’t be sunk
> 
> In particular we have some loads that need to have a constant address to be
selected, because they load from a “special address space”.
> 
> What the optimization is doing is extracting the “getelementptr” , making
an instruction out of it, sinking the load and using a PHI to select between the
address.
> This breaks our selection of this types of loads.
> 
> We need a way to tell this code to not do that.
> 
> The solutions I kind of see for this is:
> 
> - Add some TTI hook that stops this sinking from happening for instructions
that don’t support that like in this case (maybe added in canSinkInstructions())
> 
> - Add some TTI hook that is a little bit coarser that stops the
optimization to run altogether. In particular this thing is creating
“unstructured control-flow” in some cases and this is not always good (and
actually generally bad) on GPU targets.
> 
> - Put the optimization in another separate pass. It seems to be doing stuff
that is a little bit more than just “Simplifying the CFG” actually. It is
actually making it more complex by splitting blocks around. I wonder if it would
make sense for it to live it in a separate pass that can be enabled or disabled
at the compiler pipeline level so that one that wants to opt out can just avoid
to add the pass to the pipeline.
> 
> Open for suggestions on what the best solution for this is.
> 
> Thanks,
> Marcello
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160902/88267cce/attachment.html>