[PATCH] D101187: [MachineCSE] Prevent CSE of non-local convergent instrs
Roman Tereshin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 27 13:46:00 PDT 2021
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
More information about the llvm-commits
mailing list