[PATCH] D123693: Transform illegal intrinsics to V_ILLEGAL

[Brendon Cahoon via Phabricator via llvm-commits]

bcahoon added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp:4415
 
+  NODE_NAME_CASE(ILLEGAL)
+
----------------
At this location, ILLEGAL is a memory opcode, but SelectionDAGDumper will crash because when trying to print its memops.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:6633
+      if (Opcode == -1) {
+        return {};
+      }
----------------
I think it may be easiest to create the ILLEGAL instruction here rather than creating it after the call to lowerImage. That way, there doesn't have to be copies of the code that creates ILLEGAL.

For example, 
 MachineSDNode *NewNode = DAG.getMachineNode(AMDGPU::V_ILLEGAL, DL, MVT::Other, Op.getOperand(0));
 return DAG.getMergeValues({ DAG.getUNDEF(Op.getValueType()), SDValue(NewNode, 0) }, DL);

The UNDEF value replaces return result for the intrinsic, and the V_ILLEGAL replaces the chain result. It appears, though, that V_ILLEGAL is removed if the intrinsic chain result is not used anywhere.  I'm not sure if that is a bad thing since it means it's not really needed?


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:7117
+      }
+      return DAG.getNode(AMDGPUISD::ILLEGAL, DL, VTs, Ops);
+    }
----------------
Can this go in lowerImage? It seems like this will mark some cases as illegal that should be handled either in DAGToDAG or with patterns.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123693