[PATCH] D125680: Correctly legalise stackmap operands

[Edd Barrett via Phabricator via llvm-commits]

vext01 added a comment.

Responded to @arsenm's comments. Some outstanding questions.



================
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(
----------------
arsenm wrote:
> The opcode assert is implied by the cast. There's also probably no point to asserting it's i64
If the assertion fails, do we not end up doing a bogus cast and invoking undefined behaviour which will probably lead to a crash much later? Wouldn't it be best to have the assertion crash early?


================
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) {
----------------
arsenm wrote:
> arsenm wrote:
> > 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
> These parameters probably should have been marked immarg in the intrinsic definition?
> 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

Wasn't that the very reason that the constants were not being legalised? Because they were already target constants?

> These parameters probably should have been marked immarg in the intrinsic definition?

Should that be handled as a separate change, since I'm not actually touching the intrinsic's definition in this work?

I don't know how the workflow works after phabricator? Do I get an opportunity to make things into neat, self-contained commits?


================
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
+
+
----------------
arsenm wrote:
> 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
At what level should I check?

I wanted to check at the MIR level, but it doesn't show the types there.


================
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>
----------------
arsenm wrote:
> Probably should add some cases with excessively wide types, vectors and FP
Yep. I was only working on integer types for now, but I can add changes to ensure those work too.


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

https://reviews.llvm.org/D125680