[PATCH] D125680: Correctly legalise stackmap operands

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
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(
----------------
vext01 wrote:
> 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?
The point of cast<> is it does the type assertion for you


================
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) {
----------------
vext01 wrote:
> 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?
I assume these are the fixed operands at the start of the argument list, and not the variadic section. I'm also assuming legalization is only relevant for the variadic arguments.

Changing the intrinsic would be a separate change. Phabricator lets you add parent/child revisions to track related changes


================
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
+
+
----------------
vext01 wrote:
> 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.
The types are meaningless after selection, so that makes sense. I would default to codegen to the end. MIR is less stable and what you care about is that the types were legalized to and selected to something, not the types themselves


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

https://reviews.llvm.org/D125680