[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:2286
+      // Otherwise it's a register.
+      // XXX ^^ is this true?
+      O = *It; // XXX Also does this guarantee register selection?
----------------
FrameIndex will also be materialized into a register, unlike TargetFrameIndex


================
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:
> > 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.
Yes, if they needed to be target constants to begin with they should start as targetconstants


================
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:
> > 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?
Yes. No, this is a target specific test anyway. 


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

https://reviews.llvm.org/D125680