[PATCH] D67767: [AMDGPU] Add llvm.amdgcn.wqm.demote intrinsic

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

nhaehnle added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicsAMDGPU.td:1267
+// Like ps.live, but cannot be moved by LICM.
+def int_amdgcn_wqm_helper : Intrinsic <[llvm_i1_ty], [], []>;
+
----------------
critson wrote:
> arsenm wrote:
> > I assume this needs to be convergent
> Could be, my understanding is that without flags the intrinsic is marked "has side effects", which is correct as then it will not moved by LICM or removed by CSE.
Rethinking this, convergent isn't correct here, because there is no implied cross-thread communication.

Rather, the semantics are that amdgcn.wqm.helper reads from some hidden memory that is written to by wqm.demote. So by that logic, this should arguably be ReadInaccessibleMemOnly.


================
Comment at: llvm/include/llvm/IR/IntrinsicsAMDGPU.td:1485
+// If false, mark all active lanes as helper lanes until the end of program.
+def int_amdgcn_wqm_demote : Intrinsic<[], [llvm_i1_ty], []>;
+
----------------
critson wrote:
> arsenm wrote:
> > Ditto, and nomem
> Convergent maybe (as above), but not nomem.
> If this is marked nomem then it will be eaten by early CSE.
> Since this was modelled on kill, is there a reason we don't mark kill Convergent?
Following the logic above, this should not be convergent but WritesInaccessibleMemOnly. At least I *think* that captures the semantics most accurately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67767