[PATCH] D125680: Correctly legalise stackmap operands
Edd Barrett via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 4 07:00:52 PDT 2022
vext01 added inline comments.
================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:2274-2277
+ Ops.push_back(
+ CurDAG->getTargetConstant(StackMaps::ConstantOp, DL, MVT::i64));
+ O = CurDAG->getTargetConstant(
+ cast<ConstantSDNode>(OpNode)->getZExtValue(), DL, It->getValueType());
----------------
arsenm wrote:
> vext01 wrote:
> > arsenm wrote:
> > > You're still fixing this into TargetConstant at selection time instead of upfront when lowering from the IR
> > This code is selecting the stackmap live variables.
> >
> > The (non-frameindex) live variables can't be emitted to target constants at DAG-build time or they won't get legalized and that's the problem that this change is trying to address.
> >
> > Correct me if I'm wrong though.
> This is a pretty weird behavior, but does the type actually matter? Could you just unconditionally use 64-bit constants?
Sorry for the delay. I was away at the end of last week.
I've just tried to implement your suggestion, it has two undesirable effects:
- It changes the way large constant structs which get split into e.g. `mergevalues(constant, constant)` are codegenned. It appears that the constituent parts of the struct get allocated to registers and not constants.
- For FastISel, some some small constants get reported as long constants in the stackmap record.
- For FastISel It causes large constants to be emitted in a different order than in `SelectionDAGISel`, which means that the test files which test both backends cannot succeed.
This is too much scary breakage, so I suggest we emit constants as we did before.
Here is the diff (to previous) that I was working on before I abandoned it:
```
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index b9816bb34bc9..497c4c77880a 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -9449,6 +9449,10 @@ void SelectionDAGBuilder::visitStackmap(const CallInst &CI) {
const TargetLowering &TLI = DAG.getTargetLoweringInfo();
Ops.push_back(DAG.getTargetFrameIndex(
FI->getIndex(), TLI.getFrameIndexTy(DAG.getDataLayout())));
+ } else if (ConstantSDNode *C = dyn_cast<ConstantSDNode>(Op)) {
+ // Directly emit a 64-bit target constant.
+ Ops.push_back(DAG.getTargetConstant(StackMaps::ConstantOp, DL, MVT::i64));
+ Ops.push_back(DAG.getTargetConstant(C->getZExtValue(), DL, MVT::i64));
} else {
// Otherwise emit a target independent node to be legalised.
Ops.push_back(getValue(CI.getArgOperand(I)));
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
index aff4e9a94fef..936f48f34870 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
@@ -2269,15 +2269,7 @@ void SelectionDAGISel::Select_STACKMAP(SDNode *N) {
// FrameIndex nodes should have been directly emitted to TargetFrameIndex
// nodes at DAG-construction time.
assert(OpNode->getOpcode() != ISD::FrameIndex);
-
- if (OpNode->getOpcode() == ISD::Constant) {
- Ops.push_back(
- CurDAG->getTargetConstant(StackMaps::ConstantOp, DL, MVT::i64));
- O = CurDAG->getTargetConstant(
- cast<ConstantSDNode>(OpNode)->getZExtValue(), DL, It->getValueType());
- } else {
- O = *It;
- }
+ O = *It;
Ops.push_back(O);
}
diff --git a/llvm/test/CodeGen/X86/selectiondag-stackmap-legalize.ll b/llvm/test/CodeGen/X86/selectiondag-stackmap-legalize.ll
index bc624be5318e..a2d629fce00c 100644
--- a/llvm/test/CodeGen/X86/selectiondag-stackmap-legalize.ll
+++ b/llvm/test/CodeGen/X86/selectiondag-stackmap-legalize.ll
@@ -81,10 +81,10 @@
; CHECK-NEXT: .short 0
; CHECK-NEXT: .long 0
; Location[7]
-; CHECK-NEXT: .byte 4
+; CHECK-NEXT: .byte 1
; CHECK-NEXT: .byte 0
-; CHECK-NEXT: .short 8
-; CHECK-NEXT: .short 0
+; CHECK-NEXT: .short 4
+; CHECK-NEXT: .short {{.*}}
; CHECK-NEXT: .short 0
; CHECK-NEXT: .long 0
; Location[8]
@@ -95,10 +95,10 @@
; CHECK-NEXT: .short 0
; CHECK-NEXT: .long 0
; Location[9]
-; CHECK-NEXT: .byte 4
+; CHECK-NEXT: .byte 1
; CHECK-NEXT: .byte 0
-; CHECK-NEXT: .short 8
-; CHECK-NEXT: .short 0
+; CHECK-NEXT: .short 1
+; CHECK-NEXT: .short 2
; CHECK-NEXT: .short 0
; CHECK-NEXT: .long 0
; Location[10]
diff --git a/llvm/test/CodeGen/X86/stackmap-fast-isel.ll b/llvm/test/CodeGen/X86/stackmap-fast-isel.ll
index dd25065f3063..558029cdf1c5 100644
--- a/llvm/test/CodeGen/X86/stackmap-fast-isel.ll
+++ b/llvm/test/CodeGen/X86/stackmap-fast-isel.ll
@@ -29,8 +29,8 @@
; CHECK-NEXT: .quad 4
; Large Constants
-; CHECK-NEXT: .quad 2147483648
; CHECK-NEXT: .quad 4294967295
+; CHECK-NEXT: .quad 2147483648
; CHECK-NEXT: .quad 4294967296
; Callsites
@@ -46,14 +46,14 @@
; CHECK-NEXT: .short 8
; CHECK-NEXT: .short 0
; CHECK-NEXT: .short 0
-; CHECK-NEXT: .long -1
+; CHECK-NEXT: .long 65535
; SmallConstant
; CHECK-NEXT: .byte 4
; CHECK-NEXT: .byte 0
; CHECK-NEXT: .short 8
; CHECK-NEXT: .short 0
; CHECK-NEXT: .short 0
-; CHECK-NEXT: .long -1
+; CHECK-NEXT: .long 65535
; SmallConstant
; CHECK-NEXT: .byte 4
; CHECK-NEXT: .byte 0
@@ -76,19 +76,19 @@
; CHECK-NEXT: .short 0
; CHECK-NEXT: .long 2147483647
; SmallConstant
-; CHECK-NEXT: .byte 4
+; CHECK-NEXT: .byte 5
; CHECK-NEXT: .byte 0
; CHECK-NEXT: .short 8
; CHECK-NEXT: .short 0
; CHECK-NEXT: .short 0
-; CHECK-NEXT: .long -1
+; CHECK-NEXT: .long 0
; SmallConstant
-; CHECK-NEXT: .byte 4
+; CHECK-NEXT: .byte 5
; CHECK-NEXT: .byte 0
; CHECK-NEXT: .short 8
; CHECK-NEXT: .short 0
; CHECK-NEXT: .short 0
-; CHECK-NEXT: .long -1
+; CHECK-NEXT: .long 0
; SmallConstant
; CHECK-NEXT: .byte 4
; CHECK-NEXT: .byte 0
@@ -102,14 +102,14 @@
; CHECK-NEXT: .short 8
; CHECK-NEXT: .short 0
; CHECK-NEXT: .short 0
-; CHECK-NEXT: .long 0
+; CHECK-NEXT: .long 1
; LargeConstant at index 1
; CHECK-NEXT: .byte 5
; CHECK-NEXT: .byte 0
; CHECK-NEXT: .short 8
; CHECK-NEXT: .short 0
; CHECK-NEXT: .short 0
-; CHECK-NEXT: .long 1
+; CHECK-NEXT: .long 0
; LargeConstant at index 2
; CHECK-NEXT: .byte 5
; CHECK-NEXT: .byte 0
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125680/new/
https://reviews.llvm.org/D125680
More information about the llvm-commits
mailing list