[PATCH] D125680: Correctly legalise stackmap operands

[Edd Barrett via Phabricator via llvm-commits]

vext01 added a comment.

Respond to more of @arsenm's comments. Thanks.



================
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:
> 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
I had assumed that `cast<>`was an alias to `static_cast<>`, but now I see that LLVM defines it! I had no idea!

(forgive me, I'm pretty much learning C++ on the fly here)

So I agree that asserting the type of the `opCode` is redundant, but:

> There's also probably no point to asserting it's i64

How comes? Isn't it good to check the sanity of the DAG we fed in? If we remove the assertion I think the stackmap code would eventually crash somewhere a lot later in the pipeline, but wouldn't it be good to 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:
> 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
> 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.

Ah, do you mean we should emit the first two arguments directly to target nodes? Assuming so, that'd probably work and might be a little faster.

By the way, did you have any comments on the XXXs below?

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

OK. Let's do that as a separate change.


================
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:
> 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
>  I would default to codegen to the end

Do you mean to match asm code for the target architecture? Isn't that also going to be fragile?


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

https://reviews.llvm.org/D125680