[PATCH] D101187: [MachineCSE] Prevent CSE of non-local convergent instrs

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin accepted this revision.
rtereshin added a comment.

My suggestion is to keep making progress here:

1. move the check out of is profitable to processBlock top level
2. put a comprehensive comment on it outlining the issues discussed here (and off fabricator) so far
3. do (2) in the test as well (and keep the test otherwise as is)

The issues include:
a) isConvergent as of current definition in LLVM does not prove cross-block MachineCSE illegal, however, with the change MachineCSE pass takes the liberty to extend the definition of isConvergent as a practical necessity. The extension is: "assume it is illegal to make a convergent operation dependent not only on additional conditions, but also on fewer conditions than originally"
b) The current open source GPU backends as is do not appear to allow a reasonably simple test case that provably and undeniably functionally breaks w/o the MachineCSE change proposed, as a result, the test being added is merely a coverage test for the change being made, not a reproducer of an actual (execution) problem in AMDGPU backend.

And we merge it from there. This is a conditional LGTM from me, conditions are above. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101187