[PATCH] D125680: Correctly legalise stackmap operands

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp:494-495
+
+  case ISD::STACKMAP:
+    return "stackmap";
   }
----------------
I would move this above the included file (preferably in the same position as the code is defined in the enum)


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:2256-2257
+  SDValue ID = It->get();
+  assert(ID.getOpcode() == ISD::Constant);
+  assert(ID.getValueType() == MVT::i64);
+  SDValue IDConst = CurDAG->getTargetConstant(
----------------
The opcode assert is implied by the cast. There's also probably no point to asserting it's i64


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:2265-2266
+  SDValue Shad = It->get();
+  assert(Shad.getOpcode() == ISD::Constant);
+  assert(Shad.getValueType() == MVT::i32);
+  SDValue ShadConst = CurDAG->getTargetConstant(
----------------
Ditto


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:2279-2280
+          CurDAG->getTargetConstant(StackMaps::ConstantOp, DL, MVT::i64));
+      O = CurDAG->getTargetConstant(
+          cast<ConstantSDNode>(OpNode)->getZExtValue(), DL, It->getValueType());
+    } else if (OpNode->getOpcode() == ISD::FrameIndex) {
----------------
You seem to be fixing up some Constants to TargetConstant here. It would be better to just do this up front in SelectionDAGBuilder like it was before


================
Comment at: llvm/test/CodeGen/X86/selectiondag-stackmap-legalize.ll:2
+; RUN: llc -debug-only=legalize-types -print-after=finalize-isel %s -O1 -mtriple=x86_64-unknown-unknown -o /dev/null 2>&1 | FileCheck %s
+
+
----------------
If you're checking debug output you need REQUIRES: asserts. However, I don't think checking the legalizer output is the most helpful thing here. Better to check the final output


================
Comment at: llvm/test/CodeGen/X86/selectiondag-stackmap-legalize.ll:9
+  %x = icmp eq i32 %argc, 5
+  call void (i64, i32, ...) @llvm.experimental.stackmap(i64 0, i32 0, i1 %x, i7 2)
+  ; CHECK: Legalizing node: t14: ch,glue = stackmap t10, t10:1, Constant:i64<0>, Constant:i32<0>, t7, Constant:i7<2>
----------------
Probably should add some cases with excessively wide types, vectors and FP


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

https://reviews.llvm.org/D125680