[PATCH] D125680: Correctly legalise stackmap operands

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2314
+  SmallVector<SDValue> NewOps(N->ops().begin(), N->ops().end());
+  NewOps[OpNo] = ZExtPromotedInteger(N->getOperand(OpNo));
+  return SDValue(DAG.UpdateNodeOperands(N, NewOps), 0);
----------------
I'm a bit surprised this can unconditionally zero extend. I'd expect ANY_EXTEND or to have to consider some ABI property


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:5510-5517
+  if (Op->getOpcode() == ISD::Constant) {
+    // FIXME: https://github.com/llvm/llvm-project/issues/55609
+    ConstantSDNode *CN = cast<ConstantSDNode>(Op);
+    EVT Ty = Op.getValueType();
+    NewOps.push_back(
+        DAG.getTargetConstant(StackMaps::ConstantOp, DL, MVT::i64));
+    NewOps.push_back(DAG.getTargetConstant(CN->getZExtValue(), DL, Ty));
----------------
I'd rather not add a special case to make this work only for constants


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:5522-5523
+    //  - https://github.com/llvm/llvm-project/issues/55957
+    DAG.getContext()->emitError(
+        "expanding this stackmap operand is unimplemented");
+  }
----------------
My skimming of the issue says this is an issue with the lowering from IR, so why do you need to error here? In any case I think emitError should be reserved for cases where reasonable user code ran into a truly unhandleable case. It would be better to just let it fall through and hit the ordinary legalize fatal error


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:2274-2277
+      Ops.push_back(
+          CurDAG->getTargetConstant(StackMaps::ConstantOp, DL, MVT::i64));
+      O = CurDAG->getTargetConstant(
+          cast<ConstantSDNode>(OpNode)->getZExtValue(), DL, It->getValueType());
----------------
You're still fixing this into TargetConstant at selection time instead of upfront when lowering from the IR


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

https://reviews.llvm.org/D125680