[PATCH] D54516: [AMDGPU] Do not mark llvm.amdgcn.set.inactive as IntrNoMem

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

In https://reviews.llvm.org/D54516#1301197, @sameerds wrote:

> In https://reviews.llvm.org/D54516#1301072, @tpr wrote:
>
> > EarlyCSE does seem to common up in this situation. And, if I disable that, I get GVN commoning it up.
>
>
> By "disable", do you mean modifying EarlyCSE to not touch convergent calls? What if you do that for GVN as well? GVN::ValueTable::lookupOrAddCall() seems to be the right place for that. Such spot fixes as a useful step in the right direction, to be preferred over repeating the asm hack. One already exists in GVNHoist. This may sound a bit whackamoley, but the "effort" that @nhaehnle was asking about is essentially a more organized way to audit all the places where convergent calls should be handled specially. That project has not gained sufficient motivation yet to commit to.


What I mean isn't just about an audit of `convergent`. It's about **redefining** what `convergent` means. Currently, `convergent` only means "adding control-dependencies is forbidden". This means that what EarlyCSE and GVN are doing is correct.

Instead, we really want `convergent` to mean "both adding and removing control-dependencies is forbidden". Step one is to change the definition in the LangRef. Step two is the audit of making sure the code actually complies with the new definition.


Repository:
  rL LLVM

https://reviews.llvm.org/D54516