[PATCH] D154766: [GlobalISel] convergent intrinsics

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

LGTM except for the documentation sentence no longer making sense



================
Comment at: llvm/docs/GlobalISel/GenericOpcode.rst:868
 
-  Unlike SelectionDAG, there is no _VOID variant. Both of these are permitted
-  to have zero, one, or multiple results.
+  Unlike SelectionDAG, intrinsics are permitted to have zero, one, or multiple results.
+
----------------
If you drop the "is no void variant" part, the "Unlike SelectionDAG" part doesn't make sense. Either preserve the first sentence entirely or drop it


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:971
+        MF->getFunction().getContext(), static_cast<Intrinsic::ID>(IntrID));
+    bool DeclHasSideEffects = !Attrs.getMemoryEffects().doesNotAccessMemory();
+    if (NoSideEffects && DeclHasSideEffects) {
----------------
In principle we could have local call site attributes override, but it requires more work to really support 


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:4827-4829
+  auto RCP = B.buildIntrinsic(Intrinsic::amdgcn_rcp, {S32})
+                 .addUse(Mul0.getReg(0))
+                 .setMIFlags(Flags);
----------------
Really we should add buildIntrinsic that takes operand lists like other buildInstr wrappers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154766