[llvm] b172cd7 - [Statepoint] Factor out logic for non-stack non-vreg lowering [almost NFC]
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 7 13:34:40 PDT 2020
Author: Philip Reames
Date: 2020-07-07T13:34:28-07:00
New Revision: b172cd7812405beaa9db817d85ac1458f7e31547
URL: https://github.com/llvm/llvm-project/commit/b172cd7812405beaa9db817d85ac1458f7e31547
DIFF: https://github.com/llvm/llvm-project/commit/b172cd7812405beaa9db817d85ac1458f7e31547.diff
LOG: [Statepoint] Factor out logic for non-stack non-vreg lowering [almost NFC]
This is inspired by D81648. The basic idea is to have the set of SDValues which are lowered as either constants or direct frame references explicit in one place, and to separate them clearly from the spilling logic.
This is not NFC in that the handling of constants larger than > 64 bit has changed. The old lowering would crash on values which could not be encoded as a sign extended 64 bit value. The new lowering just spills all constants > 64 bits. We could be consistent about doing the sext(Con64) optimization, but I happen to know that this code path is utterly unexercised in practice, so simple is better for now.
Added:
Modified:
llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
llvm/test/CodeGen/X86/statepoint-vector.ll
Removed:
################################################################################
diff --git a/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp b/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
index 20aae87531ad..d63fedda3bd1 100644
--- a/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
@@ -221,6 +221,28 @@ static Optional<int> findPreviousSpillSlot(const Value *Val,
return None;
}
+
+/// Return true if-and-only-if the given SDValue can be lowered as either a
+/// constant argument or a stack reference. The key point is that the value
+/// doesn't need to be spilled or tracked as a vreg use.
+static bool willLowerDirectly(SDValue Incoming) {
+ // We are making an unchecked assumption that the frame size <= 2^16 as that
+ // is the largest offset which can be encoded in the stackmap format.
+ if (isa<FrameIndexSDNode>(Incoming))
+ return true;
+
+ // The largest constant describeable in the StackMap format is 64 bits.
+ // Potential Optimization: Constants values are sign extended by consumer,
+ // and thus there are many constants of static type > 64 bits whose value
+ // happens to be sext(Con64) and could thus be lowered directly.
+ if (Incoming.getValueType().getSizeInBits() > 64)
+ return false;
+
+ return (isa<ConstantSDNode>(Incoming) || isa<ConstantFPSDNode>(Incoming) ||
+ Incoming.isUndef());
+}
+
+
/// Try to find existing copies of the incoming values in stack slots used for
/// statepoint spilling. If we can find a spill slot for the incoming value,
/// mark that slot as allocated, and reuse the same slot for this safepoint.
@@ -230,12 +252,10 @@ static void reservePreviousStackSlotForValue(const Value *IncomingValue,
SelectionDAGBuilder &Builder) {
SDValue Incoming = Builder.getValue(IncomingValue);
- if (isa<ConstantSDNode>(Incoming) || isa<ConstantFPSDNode>(Incoming) ||
- isa<FrameIndexSDNode>(Incoming) || Incoming.isUndef()) {
- // We won't need to spill this, so no need to check for previously
- // allocated stack slots
+ // If we won't spill this, we don't need to check for previously allocated
+ // stack slots.
+ if (willLowerDirectly(Incoming))
return;
- }
SDValue OldLocation = Builder.StatepointLowering.getLocation(Incoming);
if (OldLocation.getNode())
@@ -384,45 +404,53 @@ lowerIncomingStatepointValue(SDValue Incoming, bool RequireSpillSlot,
SmallVectorImpl<SDValue> &Ops,
SmallVectorImpl<MachineMemOperand *> &MemRefs,
SelectionDAGBuilder &Builder) {
- // Note: We know all of these spills are independent, but don't bother to
- // exploit that chain wise. DAGCombine will happily do so as needed, so
- // doing it here would be a small compile time win at most.
- SDValue Chain = Builder.getRoot();
-
- if (Incoming.isUndef() && Incoming.getValueType().getSizeInBits() <= 64) {
- // Put an easily recognized constant that's unlikely to be a valid
- // value so that uses of undef by the consumer of the stackmap is
- // easily recognized. This is legal since the compiler is always
- // allowed to chose an arbitrary value for undef.
- pushStackMapConstant(Ops, Builder, 0xFEFEFEFE);
- return;
+
+ if (willLowerDirectly(Incoming)) {
+ if (FrameIndexSDNode *FI = dyn_cast<FrameIndexSDNode>(Incoming)) {
+ // This handles allocas as arguments to the statepoint (this is only
+ // really meaningful for a deopt value. For GC, we'd be trying to
+ // relocate the address of the alloca itself?)
+ assert(Incoming.getValueType() == Builder.getFrameIndexTy() &&
+ "Incoming value is a frame index!");
+ Ops.push_back(Builder.DAG.getTargetFrameIndex(FI->getIndex(),
+ Builder.getFrameIndexTy()));
+
+ auto &MF = Builder.DAG.getMachineFunction();
+ auto *MMO = getMachineMemOperand(MF, *FI);
+ MemRefs.push_back(MMO);
+ return;
+ }
+
+ assert(Incoming.getValueType().getSizeInBits() <= 64);
+
+ if (Incoming.isUndef()) {
+ // Put an easily recognized constant that's unlikely to be a valid
+ // value so that uses of undef by the consumer of the stackmap is
+ // easily recognized. This is legal since the compiler is always
+ // allowed to chose an arbitrary value for undef.
+ pushStackMapConstant(Ops, Builder, 0xFEFEFEFE);
+ return;
+ }
+
+ // If the original value was a constant, make sure it gets recorded as
+ // such in the stackmap. This is required so that the consumer can
+ // parse any internal format to the deopt state. It also handles null
+ // pointers and other constant pointers in GC states.
+ if (ConstantSDNode *C = dyn_cast<ConstantSDNode>(Incoming)) {
+ pushStackMapConstant(Ops, Builder, C->getSExtValue());
+ return;
+ } else if (ConstantFPSDNode *C = dyn_cast<ConstantFPSDNode>(Incoming)) {
+ pushStackMapConstant(Ops, Builder,
+ C->getValueAPF().bitcastToAPInt().getZExtValue());
+ return;
+ }
+
+ llvm_unreachable("unhandled direct lowering case");
}
- // If the original value was a constant, make sure it gets recorded as
- // such in the stackmap. This is required so that the consumer can
- // parse any internal format to the deopt state. It also handles null
- // pointers and other constant pointers in GC states. Note the constant
- // vectors do not appear to actually hit this path and that anything larger
- // than an i64 value (not type!) will fail asserts here.
- if (ConstantFPSDNode *C = dyn_cast<ConstantFPSDNode>(Incoming)) {
- pushStackMapConstant(Ops, Builder,
- C->getValueAPF().bitcastToAPInt().getZExtValue());
- } else if (ConstantSDNode *C = dyn_cast<ConstantSDNode>(Incoming)) {
- pushStackMapConstant(Ops, Builder, C->getSExtValue());
- } else if (FrameIndexSDNode *FI = dyn_cast<FrameIndexSDNode>(Incoming)) {
- // This handles allocas as arguments to the statepoint (this is only
- // really meaningful for a deopt value. For GC, we'd be trying to
- // relocate the address of the alloca itself?)
- assert(Incoming.getValueType() == Builder.getFrameIndexTy() &&
- "Incoming value is a frame index!");
- Ops.push_back(Builder.DAG.getTargetFrameIndex(FI->getIndex(),
- Builder.getFrameIndexTy()));
- auto &MF = Builder.DAG.getMachineFunction();
- auto *MMO = getMachineMemOperand(MF, *FI);
- MemRefs.push_back(MMO);
- } else if (!RequireSpillSlot) {
+ if (!RequireSpillSlot) {
// If this value is live in (not live-on-return, or live-through), we can
// treat it the same way patchpoint treats it's "live in" values. We'll
// end up folding some of these into stack references, but they'll be
@@ -432,20 +460,20 @@ lowerIncomingStatepointValue(SDValue Incoming, bool RequireSpillSlot,
// fix-up pass should be executed to force spilling of such registers.
Ops.push_back(Incoming);
} else {
- // Otherwise, locate a spill slot and explicitly spill it so it
- // can be found by the runtime later. We currently do not support
- // tracking values through callee saved registers to their eventual
- // spill location. This would be a useful optimization, but would
- // need to be optional since it requires a lot of complexity on the
- // runtime side which not all would support.
+ // Otherwise, locate a spill slot and explicitly spill it so it can be
+ // found by the runtime later. Note: We know all of these spills are
+ // independent, but don't bother to exploit that chain wise. DAGCombine
+ // will happily do so as needed, so doing it here would be a small compile
+ // time win at most.
+ SDValue Chain = Builder.getRoot();
auto Res = spillIncomingStatepointValue(Incoming, Chain, Builder);
Ops.push_back(std::get<0>(Res));
if (auto *MMO = std::get<2>(Res))
MemRefs.push_back(MMO);
Chain = std::get<1>(Res);;
+ Builder.DAG.setRoot(Chain);
}
- Builder.DAG.setRoot(Chain);
}
/// Lower deopt state and gc pointer arguments of the statepoint. The actual
diff --git a/llvm/test/CodeGen/X86/statepoint-vector.ll b/llvm/test/CodeGen/X86/statepoint-vector.ll
index 3c2408251d6c..a7d7be8ed069 100644
--- a/llvm/test/CodeGen/X86/statepoint-vector.ll
+++ b/llvm/test/CodeGen/X86/statepoint-vector.ll
@@ -112,21 +112,26 @@ entry:
ret <2 x i8 addrspace(1)*> %obj.relocated
}
-; Check that we can lower a constant typed as i128 correctly. Note that the
-; actual value is representable in 64 bits. We don't have a representation
-; of larger than 64 bit constant in the StackMap format.
+; Check that we can lower a constant typed as i128 correctly. We don't have
+; a representation of larger than 64 bit constant in the StackMap format. At
+; the moment, this simply means spilling them, but there's a potential
+; optimization for values representable as sext(Con64).
define void @test5() gc "statepoint-example" {
; CHECK-LABEL: test5:
; CHECK: # %bb.0: # %entry
-; CHECK-NEXT: pushq %rax
-; CHECK-NEXT: .cfi_def_cfa_offset 16
+; CHECK-NEXT: subq $40, %rsp
+; CHECK-NEXT: .cfi_def_cfa_offset 48
+; CHECK-NEXT: xorps %xmm0, %xmm0
+; CHECK-NEXT: movups %xmm0, {{[0-9]+}}(%rsp)
+; CHECK-NEXT: movq $-1, {{[0-9]+}}(%rsp)
+; CHECK-NEXT: movq $-1, {{[0-9]+}}(%rsp)
; CHECK-NEXT: callq do_safepoint
; CHECK-NEXT: .Ltmp4:
-; CHECK-NEXT: popq %rax
+; CHECK-NEXT: addq $40, %rsp
; CHECK-NEXT: .cfi_def_cfa_offset 8
; CHECK-NEXT: retq
entry:
- %safepoint_token = call token (i64, i32, void ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 0, i32 0, void ()* @do_safepoint, i32 0, i32 0, i32 0, i32 0) ["deopt" (i128 0)]
+ %safepoint_token = call token (i64, i32, void ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 0, i32 0, void ()* @do_safepoint, i32 0, i32 0, i32 0, i32 0) ["deopt" (i128 0, i128 -1)]
ret void
}
More information about the llvm-commits
mailing list