[PATCH] D118250: AMDGPU: Mark control flow intrinsics non-duplicable

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

In D118250#3272547 <https://reviews.llvm.org/D118250#3272547>, @ruiling wrote:

> In D118250#3272530 <https://reviews.llvm.org/D118250#3272530>, @arsenm wrote:
>
>> I don't think this is a good idea. We don't actually need a structured CFG at this point, and tail duplicating isn't exactly unstructuring anyway. This is not an alternative to fixing the LiveVariables update problem, it's just the testcase that broke happened to have appeared due to tail duplication
>
> Tail duplicating divergent branching is also pretty bad idea. Can you prove ANY benefit by duplicating divergent branching?

Structurization is an aid to insert the exec mask manipulation instructions, and after that point we no longer care about preserving it. SI_IF isn't really a branch, although we insert a skip exec branch just in case it's necessary and is logically just bit manipulation. It's the ugly glue we use between the real CFG and the divergent CFG.

In D118250#3272547 <https://reviews.llvm.org/D118250#3272547>, @ruiling wrote:

> In D118250#3272530 <https://reviews.llvm.org/D118250#3272530>, @arsenm wrote:
>
>> I don't think this is a good idea. We don't actually need a structured CFG at this point, and tail duplicating isn't exactly unstructuring anyway. This is not an alternative to fixing the LiveVariables update problem, it's just the testcase that broke happened to have appeared due to tail duplication
>
> Tail duplicating divergent branching is also pretty bad idea. Can you prove ANY benefit by duplicating divergent branching?

It's probably not a good idea, but we don't really require this property. In the testcase I was looking at, it nets adding one additional instruction (probably because it ends up confusing the optimize exec passes). I guess given how infrequently this probably occurs, this is OK. I think it would be an interesting experiment to run benchmarks with and without this with global-isel (although we probably need to have correct regbankselect first)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118250