[ExtendLifetimes] Implement llvm.fake.use to extend variable lifetimes #86149
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
backend:X86
llvm:support
llvm:SelectionDAG
|
@llvm/pr-subscribers-backend-spir-v @llvm/pr-subscribers-llvm-ir Author: Stephen Tozer (SLTozer) ChangesThis patch is part of a set of patches that add an This patch implements a new intrinsic instruction in LLVM, Patch is 75.72 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/86149.diff 46 Files Affected:
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index 8bc1cab01bf0a6..e7b8af3a2116c7 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -27896,6 +27896,42 @@ execution, but is unknown at compile time.
If the result value does not fit in the result type, then the result is
a :ref:`poison value <poisonvalues>`.
+.. _llvm_fake_use:
+
+'``llvm.fake.use``' Intrinsic
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Syntax:
+"""""""
+
+::
+
+ declare void @llvm.fake.use(...)
+
+Overview:
+"""""""""
+
+The ``llvm.fake.use`` intrinsic is a no-op. It takes a single
+value as an operand and is treated as a use of that operand, to force the
+optimizer to preserve that value prior to the fake use. This is used for
+extending the lifetimes of variables, where this intrinsic placed at the end of
+a variable's scope helps prevent that variable from being optimized out.
+
+Arguments:
+""""""""""
+
+The ``llvm.fake.use`` intrinsic takes one argument, which may be any
+function-local SSA value. Note that the signature is variadic so that the
+intrinsic can take any type of argument, but passing more than one argument will
+result in an error.
+
+Semantics:
+""""""""""
+
+This intrinsic does nothing, but optimizers must consider it a use of its single
+operand and should try to preserve the intrinsic and its position in the
+function.
+
Stack Map Intrinsics
--------------------
diff --git a/llvm/include/llvm/Analysis/PtrUseVisitor.h b/llvm/include/llvm/Analysis/PtrUseVisitor.h
index 86206b2d5e9f88..4e46d441fce17e 100644
--- a/llvm/include/llvm/Analysis/PtrUseVisitor.h
+++ b/llvm/include/llvm/Analysis/PtrUseVisitor.h
@@ -279,6 +279,7 @@ class PtrUseVisitor : protected InstVisitor<DerivedT>,
default:
return Base::visitIntrinsicInst(II);
+ case Intrinsic::fake_use:
case Intrinsic::lifetime_start:
case Intrinsic::lifetime_end:
return; // No-op intrinsics.
diff --git a/llvm/include/llvm/CodeGen/ISDOpcodes.h b/llvm/include/llvm/CodeGen/ISDOpcodes.h
index 49d51a27e3c0f6..43aa02b5b03762 100644
--- a/llvm/include/llvm/CodeGen/ISDOpcodes.h
+++ b/llvm/include/llvm/CodeGen/ISDOpcodes.h
@@ -1304,6 +1304,11 @@ enum NodeType {
LIFETIME_START,
LIFETIME_END,
+ /// FAKE_USE represents a use of the operand but does not do anything.
+ /// Its purpose is the extension of the operand's lifetime mainly for
+ /// debugging purposes.
+ FAKE_USE,
+
/// GC_TRANSITION_START/GC_TRANSITION_END - These operators mark the
/// beginning and end of GC transition sequence, and carry arbitrary
/// information that target might need for lowering. The first operand is
diff --git a/llvm/include/llvm/CodeGen/MachineInstr.h b/llvm/include/llvm/CodeGen/MachineInstr.h
index fcdd73d8b65fdd..163bcf6aa5a1a2 100644
--- a/llvm/include/llvm/CodeGen/MachineInstr.h
+++ b/llvm/include/llvm/CodeGen/MachineInstr.h
@@ -1401,6 +1401,8 @@ class MachineInstr
return getOpcode() == TargetOpcode::EXTRACT_SUBREG;
}
+ bool isFakeUse() const { return getOpcode() == TargetOpcode::FAKE_USE; }
+
/// Return true if the instruction behaves like a copy.
/// This does not include native copy instructions.
bool isCopyLike() const {
diff --git a/llvm/include/llvm/CodeGen/SelectionDAGISel.h b/llvm/include/llvm/CodeGen/SelectionDAGISel.h
index 837f8bf7263ea9..cb8dec554a57b6 100644
--- a/llvm/include/llvm/CodeGen/SelectionDAGISel.h
+++ b/llvm/include/llvm/CodeGen/SelectionDAGISel.h
@@ -453,6 +453,7 @@ class SelectionDAGISel : public MachineFunctionPass {
void Select_READ_REGISTER(SDNode *Op);
void Select_WRITE_REGISTER(SDNode *Op);
void Select_UNDEF(SDNode *N);
+ void Select_FAKE_USE(SDNode *N);
void CannotYetSelect(SDNode *N);
void Select_FREEZE(SDNode *N);
diff --git a/llvm/include/llvm/IR/Intrinsics.td b/llvm/include/llvm/IR/Intrinsics.td
index 091f9b38107989..23a9f586056267 100644
--- a/llvm/include/llvm/IR/Intrinsics.td
+++ b/llvm/include/llvm/IR/Intrinsics.td
@@ -1778,6 +1778,9 @@ def int_is_constant : DefaultAttrsIntrinsic<[llvm_i1_ty], [llvm_any_ty],
[IntrNoMem, IntrWillReturn, IntrConvergent],
"llvm.is.constant">;
+// Introduce a use of the argument without generating any code.
+def int_fake_use : Intrinsic<[], [llvm_vararg_ty]>;
+
// Intrinsic to mask out bits of a pointer.
// First argument must be pointer or vector of pointer. This is checked by the
// verifier.
diff --git a/llvm/include/llvm/Support/TargetOpcodes.def b/llvm/include/llvm/Support/TargetOpcodes.def
index 899eaad5842ae0..9797bbe02a9b5c 100644
--- a/llvm/include/llvm/Support/TargetOpcodes.def
+++ b/llvm/include/llvm/Support/TargetOpcodes.def
@@ -217,6 +217,9 @@ HANDLE_TARGET_OPCODE(PATCHABLE_TYPED_EVENT_CALL)
HANDLE_TARGET_OPCODE(ICALL_BRANCH_FUNNEL)
+/// Represents a use of the operand but generates no code.
+HANDLE_TARGET_OPCODE(FAKE_USE)
+
// This is a fence with the singlethread scope. It represents a compiler memory
// barrier, but does not correspond to any generated instruction.
HANDLE_TARGET_OPCODE(MEMBARRIER)
diff --git a/llvm/include/llvm/Target/Target.td b/llvm/include/llvm/Target/Target.td
index cb1c0ed2513d45..2f5d9a5b8ea877 100644
--- a/llvm/include/llvm/Target/Target.td
+++ b/llvm/include/llvm/Target/Target.td
@@ -1401,6 +1401,14 @@ def FAULTING_OP : StandardPseudoInstruction {
let isTerminator = true;
let isBranch = true;
}
+def FAKE_USE : StandardPseudoInstruction {
+ // An instruction that uses its operands but does nothing.
+ let OutOperandList = (outs);
+ let InOperandList = (ins variable_ops);
+ let AsmString = "FAKE_USE";
+ let hasSideEffects = 0;
+ let isMeta = true;
+}
def PATCHABLE_OP : StandardPseudoInstruction {
let OutOperandList = (outs);
let InOperandList = (ins variable_ops);
diff --git a/llvm/lib/CodeGen/Analysis.cpp b/llvm/lib/CodeGen/Analysis.cpp
index af7643d93591f7..05840c963ea526 100644
--- a/llvm/lib/CodeGen/Analysis.cpp
+++ b/llvm/lib/CodeGen/Analysis.cpp
@@ -566,7 +566,8 @@ bool llvm::isInTailCallPosition(const CallBase &Call, const TargetMachine &TM) {
if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(BBI))
if (II->getIntrinsicID() == Intrinsic::lifetime_end ||
II->getIntrinsicID() == Intrinsic::assume ||
- II->getIntrinsicID() == Intrinsic::experimental_noalias_scope_decl)
+ II->getIntrinsicID() == Intrinsic::experimental_noalias_scope_decl ||
+ II->getIntrinsicID() == Intrinsic::fake_use)
continue;
if (BBI->mayHaveSideEffects() || BBI->mayReadFromMemory() ||
!isSafeToSpeculativelyExecute(&*BBI))
diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
index a15538755d73b3..1e680630e6cb58 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -1099,11 +1099,46 @@ void AsmPrinter::emitFunctionEntryLabel() {
}
}
+// Recognize cases where a spilled register is reloaded solely to feed into a
+// FAKE_USE.
+static bool isLoadFeedingIntoFakeUse(const MachineInstr &MI) {
+ const MachineFunction *MF = MI.getMF();
+ const TargetInstrInfo *TII = MF->getSubtarget().getInstrInfo();
+
+ // If the restore size is std::nullopt then we are not dealing with a reload
+ // of a spilled register.
+ if (!MI.getRestoreSize(TII))
+ return false;
+
+ // Check if this register is the operand of a FAKE_USE and
+ // does it have the kill flag set there.
+ auto NextI = std::next(MI.getIterator());
+ if (NextI == MI.getParent()->end() || !NextI->isFakeUse())
+ return false;
+
+ unsigned Reg = MI.getOperand(0).getReg();
+ for (const MachineOperand &MO : NextI->operands()) {
+ // Return true if we came across the register from the
+ // previous spill instruction that is killed in NextI.
+ if (MO.isReg() && MO.isUse() && MO.isKill() && MO.getReg() == Reg)
+ return true;
+ }
+
+ return false;
+}
+
/// emitComments - Pretty-print comments for instructions.
static void emitComments(const MachineInstr &MI, raw_ostream &CommentOS) {
const MachineFunction *MF = MI.getMF();
const TargetInstrInfo *TII = MF->getSubtarget().getInstrInfo();
+ // If this is a reload of a spilled register that only feeds into a FAKE_USE
+ // instruction, meaning the load value has no effect on the program and has
+ // only been kept alive for debugging; since it is still available on the
+ // stack, we can skip the load itself.
+ if (isLoadFeedingIntoFakeUse(MI))
+ return;
+
// Check for spills and reloads
// We assume a single instruction only has a spill or reload, not
@@ -1828,6 +1863,8 @@ void AsmPrinter::emitFunctionBody() {
case TargetOpcode::KILL:
if (isVerbose()) emitKill(&MI, *this);
break;
+ case TargetOpcode::FAKE_USE:
+ break;
case TargetOpcode::PSEUDO_PROBE:
emitPseudoProbe(MI);
break;
@@ -1843,6 +1880,12 @@ void AsmPrinter::emitFunctionBody() {
// purely meta information.
break;
default:
+ // If this is a reload of a spilled register that only feeds into a
+ // FAKE_USE instruction, meaning the load value has no effect on the
+ // program and has only been kept alive for debugging; since it is
+ // still available on the stack, we can skip the load itself.
+ if (isLoadFeedingIntoFakeUse(MI))
+ break;
emitInstruction(&MI);
if (CanDoExtraAnalysis) {
MCInst MCI;
diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index 9f99bb7e693f7e..a2314779c8e687 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -2663,12 +2663,34 @@ bool CodeGenPrepare::dupRetToEnableTailCallOpts(BasicBlock *BB,
return false;
};
+ SmallVector<const IntrinsicInst *, 4> FakeUses;
+
+ auto isFakeUse = [&FakeUses](const Instruction *Inst) {
+ if (auto *II = dyn_cast<IntrinsicInst>(Inst);
+ II && II->getIntrinsicID() == Intrinsic::fake_use) {
+ // Record the instruction so it can be preserved when the exit block is
+ // removed. Do not preserve the fake use that uses the result of the
+ // PHI instruction.
+ // Do not copy fake uses that use the result of a PHI node.
+ // FIXME: If we do want to copy the fake use into the return blocks, we
+ // have to figure out which of the PHI node operands to use for each
+ // copy.
+ if (!isa<PHINode>(II->getOperand(0))) {
+ FakeUses.push_back(II);
+ }
+ return true;
+ }
+
+ return false;
+ };
+
// Make sure there are no instructions between the first instruction
// and return.
const Instruction *BI = BB->getFirstNonPHI();
// Skip over debug and the bitcast.
while (isa<DbgInfoIntrinsic>(BI) || BI == BCI || BI == EVI ||
- isa<PseudoProbeInst>(BI) || isLifetimeEndOrBitCastFor(BI))
+ isa<PseudoProbeInst>(BI) || isLifetimeEndOrBitCastFor(BI) ||
+ isFakeUse(BI))
BI = BI->getNextNode();
if (BI != RetI)
return false;
@@ -2677,6 +2699,7 @@ bool CodeGenPrepare::dupRetToEnableTailCallOpts(BasicBlock *BB,
/// call.
const Function *F = BB->getParent();
SmallVector<BasicBlock *, 4> TailCallBBs;
+ SmallVector<CallInst *, 4> CallInsts;
if (PN) {
for (unsigned I = 0, E = PN->getNumIncomingValues(); I != E; ++I) {
// Look through bitcasts.
@@ -2710,6 +2733,9 @@ bool CodeGenPrepare::dupRetToEnableTailCallOpts(BasicBlock *BB,
attributesPermitTailCall(F, CI, RetI, *TLI))
TailCallBBs.push_back(PredBB);
}
+ // Record the call instruction so we can insert any fake uses
+ // that need to be preserved before it.
+ CallInsts.push_back(CI);
}
} else {
SmallPtrSet<BasicBlock *, 4> VisitedBBs;
@@ -2726,6 +2752,9 @@ bool CodeGenPrepare::dupRetToEnableTailCallOpts(BasicBlock *BB,
(isIntrinsicOrLFToBeTailCalled(TLInfo, CI) &&
V == CI->getArgOperand(0))) {
TailCallBBs.push_back(Pred);
+ // Record the call instruction so we can insert any fake uses
+ // that need to be preserved before it.
+ CallInsts.push_back(CI);
}
}
}
@@ -2752,8 +2781,17 @@ bool CodeGenPrepare::dupRetToEnableTailCallOpts(BasicBlock *BB,
}
// If we eliminated all predecessors of the block, delete the block now.
- if (Changed && !BB->hasAddressTaken() && pred_empty(BB))
+ if (Changed && !BB->hasAddressTaken() && pred_empty(BB)) {
+ // Copy the fake uses found in the original return block to all blocks
+ // that contain tail calls.
+ for (auto *CI : CallInsts) {
+ for (auto const *FakeUse : FakeUses) {
+ auto *ClonedInst = FakeUse->clone();
+ ClonedInst->insertBefore(CI);
+ }
+ }
BB->eraseFromParent();
+ }
return Changed;
}
diff --git a/llvm/lib/CodeGen/DeadMachineInstructionElim.cpp b/llvm/lib/CodeGen/DeadMachineInstructionElim.cpp
index facc01452d2f12..468949fd4cf498 100644
--- a/llvm/lib/CodeGen/DeadMachineInstructionElim.cpp
+++ b/llvm/lib/CodeGen/DeadMachineInstructionElim.cpp
@@ -87,7 +87,8 @@ bool DeadMachineInstructionElimImpl::isDead(const MachineInstr *MI) const {
return false;
// Don't delete frame allocation labels.
- if (MI->getOpcode() == TargetOpcode::LOCAL_ESCAPE)
+ if (MI->getOpcode() == TargetOpcode::LOCAL_ESCAPE ||
+ MI->getOpcode() == TargetOpcode::FAKE_USE)
return false;
// Don't delete instructions with side effects.
diff --git a/llvm/lib/CodeGen/MachineCSE.cpp b/llvm/lib/CodeGen/MachineCSE.cpp
index 26a8d00e662651..c3c7f48677d5c2 100644
--- a/llvm/lib/CodeGen/MachineCSE.cpp
+++ b/llvm/lib/CodeGen/MachineCSE.cpp
@@ -406,7 +406,7 @@ bool MachineCSE::PhysRegDefsReach(MachineInstr *CSMI, MachineInstr *MI,
bool MachineCSE::isCSECandidate(MachineInstr *MI) {
if (MI->isPosition() || MI->isPHI() || MI->isImplicitDef() || MI->isKill() ||
- MI->isInlineAsm() || MI->isDebugInstr() || MI->isJumpTableDebugInfo())
+ MI->isInlineAsm() || MI->isDebugInstr() || MI->isJumpTableDebugInfo() || MI->isFakeUse())
return false;
// Ignore copies.
diff --git a/llvm/lib/CodeGen/MachineScheduler.cpp b/llvm/lib/CodeGen/MachineScheduler.cpp
index 0d5bf329938781..1cc611107e5984 100644
--- a/llvm/lib/CodeGen/MachineScheduler.cpp
+++ b/llvm/lib/CodeGen/MachineScheduler.cpp
@@ -524,7 +524,8 @@ static bool isSchedBoundary(MachineBasicBlock::iterator MI,
MachineBasicBlock *MBB,
MachineFunction *MF,
const TargetInstrInfo *TII) {
- return MI->isCall() || TII->isSchedulingBoundary(*MI, MBB, *MF);
+ return MI->isCall() || TII->isSchedulingBoundary(*MI, MBB, *MF) ||
+ MI->isFakeUse();
}
/// A region of an MBB for scheduling.
diff --git a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
index 27b8472ddb73d8..5451abcdfcf9d2 100644
--- a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
@@ -1461,6 +1461,9 @@ bool FastISel::selectIntrinsicCall(const IntrinsicInst *II) {
updateValueMap(II, ResultReg);
return true;
}
+ case Intrinsic::fake_use:
+ // At -O0, we don't need fake use, so just ignore it.
+ return true;
case Intrinsic::experimental_stackmap:
return selectStackmap(II);
case Intrinsic::experimental_patchpoint_void:
diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
index 3332c02ec72358..ac1242ff59f164 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
@@ -2229,6 +2229,9 @@ bool DAGTypeLegalizer::PromoteFloatOperand(SDNode *N, unsigned OpNo) {
report_fatal_error("Do not know how to promote this operator's operand!");
case ISD::BITCAST: R = PromoteFloatOp_BITCAST(N, OpNo); break;
+ case ISD::FAKE_USE:
+ R = PromoteFloatOp_FAKE_USE(N, OpNo);
+ break;
case ISD::FCOPYSIGN: R = PromoteFloatOp_FCOPYSIGN(N, OpNo); break;
case ISD::FP_TO_SINT:
case ISD::FP_TO_UINT:
@@ -2268,6 +2271,13 @@ SDValue DAGTypeLegalizer::PromoteFloatOp_BITCAST(SDNode *N, unsigned OpNo) {
return DAG.getBitcast(N->getValueType(0), Convert);
}
+SDValue DAGTypeLegalizer::PromoteFloatOp_FAKE_USE(SDNode *N, unsigned OpNo) {
+ assert(OpNo == 1 && "Only Operand 1 must need promotion here");
+ SDValue Op = GetPromotedFloat(N->getOperand(OpNo));
+ return DAG.getNode(N->getOpcode(), SDLoc(N), MVT::Other, N->getOperand(0),
+ Op);
+}
+
// Promote Operand 1 of FCOPYSIGN. Operand 0 ought to be handled by
// PromoteFloatRes_FCOPYSIGN.
SDValue DAGTypeLegalizer::PromoteFloatOp_FCOPYSIGN(SDNode *N, unsigned OpNo) {
@@ -3138,6 +3148,9 @@ bool DAGTypeLegalizer::SoftPromoteHalfOperand(SDNode *N, unsigned OpNo) {
"operand!");
case ISD::BITCAST: Res = SoftPromoteHalfOp_BITCAST(N); break;
+ case ISD::FAKE_USE:
+ Res = SoftPromoteHalfOp_FAKE_USE(N, OpNo);
+ break;
case ISD::FCOPYSIGN: Res = SoftPromoteHalfOp_FCOPYSIGN(N, OpNo); break;
case ISD::FP_TO_SINT:
case ISD::FP_TO_UINT: Res = SoftPromoteHalfOp_FP_TO_XINT(N); break;
@@ -3175,6 +3188,13 @@ SDValue DAGTypeLegalizer::SoftPromoteHalfOp_BITCAST(SDNode *N) {
return DAG.getNode(ISD::BITCAST, SDLoc(N), N->getValueType(0), Op0);
}
+SDValue DAGTypeLegalizer::SoftPromoteHalfOp_FAKE_USE(SDNode *N, unsigned OpNo) {
+ assert(OpNo == 1 && "Only Operand 1 must need promotion here");
+ SDValue Op = GetSoftPromotedHalf(N->getOperand(OpNo));
+ return DAG.getNode(N->getOpcode(), SDLoc(N), MVT::Other, N->getOperand(0),
+ Op);
+}
+
SDValue DAGTypeLegalizer::SoftPromoteHalfOp_FCOPYSIGN(SDNode *N,
unsigned OpNo) {
assert(OpNo == 1 && "Only Operand 1 must need promotion here");
diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
index 93ce9c22af5525..022cb9a4a82cdc 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
@@ -1818,6 +1818,9 @@ bool DAGTypeLegalizer::PromoteIntegerOperand(SDNode *N, unsigned OpNo) {
case ISD::BUILD_VECTOR: Res = PromoteIntOp_BUILD_VECTOR(N); break;
case ISD::CONCAT_VECTORS: Res = PromoteIntOp_CONCAT_VECTORS(N); break;
case ISD::EXTRACT_VECTOR_ELT: Res = PromoteIntOp_EXTRACT_VECTOR_ELT(N); break;
+ case ISD::FAKE_USE:
+ Res = PromoteIntOp_FAKE_USE(N);
+ break;
case ISD::INSERT_VECTOR_ELT:
Res = PromoteIntOp_INSERT_VECTOR_ELT(N, OpNo);
break;
@@ -5124,6 +5127,9 @@ bool DAGTypeLegalizer::ExpandIntegerOperand(SDNode *N, unsigned OpNo) {
case ISD::BR_CC: Res = ExpandIntOp_BR_CC(N); break;
case ISD::BUILD_VECTOR: Res = ExpandOp_BUILD_VECTOR(N); break;
case ISD::EXTRACT_ELEMENT: Res = ExpandOp_EXTRACT_ELEMENT(N); break;
+ case ISD::FAKE_USE:
+ Res = ExpandOp_FAKE_USE(N);
+ break;
case ISD::INSERT_VECTOR_ELT: Res = ExpandOp_INSERT_VECTOR_ELT(N); break;
case ISD::SCALAR_TO_VECTOR: Res = ExpandOp_SCALAR_TO_VECTOR(N); break;
case ISD::SPLAT_VECTOR: Res = ExpandIntOp_SPLAT_VECTOR(N); break;
@@ -5931,6 +5937,19 @@ SDValue DAGTypeLegalizer::PromoteIntOp_INSERT_SUBVECTOR(SDNode *N) {
return DAG.getAnyExtOrTrunc(Ext, dl, N->getValueType(0));
}
+// FIXME: We wouldn't need this if clang could promote short integers
+// that are arguments to FAKE_USE.
+SDValue DAGTypeLegalizer::PromoteIntOp_FAKE_USE(SDNode *N) {
+ SDLoc dl(N);
+ SDValue V0 = N->getOperand(0);
+ SDValue V1 = N->getOperand(1);
+ EVT InVT1 = V1.getValueType();
+ SDValue VPromoted =
+ DAG.getNode(ISD::ANY_EXTEND, dl,
+ TLI.getTypeToTransformTo(*DAG.getContext(), InVT1), V1);
+ return DAG.getNode(N->getOpcode(), dl, N->getValueType(0), V0, VPromoted);
+}
+
SDValue DAGTypeLegalizer::PromoteIntOp_EXTRACT_SUBVECTOR(SDNode *N) {
SDLoc dl(N);
SDValue V0 = GetPromotedInteger(N->getOperand(0));
diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h b/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
index e08acd36b41d4e..6a3f76ae4b6ded 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
+++ b/llvm/lib/CodeG...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
I only skimmed the diff, but noticed there is no global isel lowering here. Were there any challenges in doing that or was it not considered? |
Our target of interest is X86, so I believe it was not considered. @wolfy1961 can correct me if I'm mis-remembering. |
I wasn't the original patch writer, but I think it would be the latter. I'm not familiar with the internals of GlobalISel, so I'm not sure how confident I'd be implementing support for fake use there myself - though FWIW, since it's a no-op it's mostly just filling in the boilerplate, with a few more complex cases (such as moving fake uses before tail calls). |
The reason I asked is -- and I could be wrong -- that GlobalISel will give up and fall back to SelectionDAG when it seems something it doesn't know how to lower. That usually happens in IRTranslator.cpp: So, depending on how/when these intrinsics are generated, this patch could affect a few different targets. edit: not this patch, but the one that would generate the intrinsics |
And it would introduce a situation we usually try to avoid: the presence of debug info could significantly change code generation. |
IIRC, the original patch predated Global Isel, or else it was in a very early stage at the time, so it was simply not considered.
The patch wasn't meant to be specific to any target, although our efforts were concentrating on X86. |
Interesting; I think falling back on SelectionDAG would produce a relatively normal result (emitting
This intrinsic has no technical relation to debug info; the purpose is to improve debug info quality when enabled, but the codegen effects of this flag are unrelated to whether debug info is emitted or not; if you use |
Conceptually, though, extend-lifetimes is purely a codegen change. It could stand on its own, even though it would only be useful to improve debuggability. I think it's important to separate the 2 concepts. Debug info generation by itself does not trigger extend-lifetimes unless we specifically also request extend-lifetimes as well. |
To clarify, the entire function would be skipped by GlobalISEL, not just the lowering of one instruction. |
Isn't the following statement contradicting the above?
|
Ah, I misunderstood - in which case forcing it to be dropped until a complete implementation is present would be preferable.
|
|
Passing through a pseudo through globalisel should be trivial; it will just need legalization handling approximately equivalent to whatever every target does for G_IMPLICIT_DEF |
arsenm
reviewed
Mar 22, 2024
OCHyams
reviewed
Apr 25, 2024
There was a problem hiding this comment.
I've looked over the code and test changes. The contents and style LGTM overall (some minor inline comments). I'm not familiar with the legalizer or X86FloatingPoint.cpp, so if someone could take a look at those parts specifically that would be great (or @arsenm have you looked at those bits already?).
Once that has been done, and once GlobalISel support has been added, I think that's everything covered by review.
I also work at Sony so I'm hesitant to Accept this once those bits have been done (I know that I am "allowed" to, but for a larger feature it feels right if others are involved too).
For added confidence, it should also be noted that the functionality has been working well for us downstream for over 5 years (possibly 7 years, but I haven't done much archeology on it) - we work at tot and we've been maintaining it since, so we know it works with today's LLVM. The feature was first mentioned upstream all the way back in 2015, here https://lists.llvm.org/pipermail/llvm-dev/2015-September/090610.html. That is all to say, as far as "new" features go this one has gone through a lot of real-world testing.
The feature this patch supports (-fextend-lifetimes, and improving the -Og pipeline to bring substantial debugging benefits) was discussed at EuroLLVM 2024 - many people seemed to support this idea and I didn't see anyone one raised any objection to it.
| # We make sure that, beginning with the first FAKE_USE instruction, | ||
| # no changes to the sequence of instructions are undertaken by the | ||
| # scheduler. We don't bother to check that the order of the FAKE_USEs | ||
| # remains the same. They should, but it is irrelevant. |
There was a problem hiding this comment.
I feels like this test has the potential to be able to be a rotten green test (i.e., what's to say that the scheduler would move things past the FAKE_USE anyway?). Possibly solvable with some creative sed-ing? I'm unsure.
| ; REQUIRES: object-emission | ||
|
|
||
| ; Make sure the fake use of 'b' at the end of 'foo' causes location information for 'b' | ||
| ; to extend all the way to the end of the function. |
There was a problem hiding this comment.
In an ideal world, we'd probably want to also check that not-using fake-uses results in b not extending to the end of the function, to prevent rotten green tests. Might be a bit fiddly though. wdyt, is it possible?
There was a problem hiding this comment.
Making something that pretty firmly forces optimizations that would reuse the storage shouldn't be too hard...
| @@ -0,0 +1,100 @@ | |||
| # Parsing dwarfdump's output to determine whether the location list for the | |||
There was a problem hiding this comment.
Maybe I'm not fully understanding this use case, but can't we replicate these checks with filecheck instead? IMO that would be cleaner than introducing some special equipment, if we can avoid it. Wdyt?
There was a problem hiding this comment.
I agree in principle, the reason it's done here though is that the fake use can't guarantee liveness to the very end of the function, and so we can correctly lose "b" before a copy+return at the end of the function. The script adjusts for this by allowing a small numeric difference between the end of b's live range and the subprogram high PC; the way that I've updated the test to not be rotten means that right now b's live range extends to the end of the function, so if that's good enough for now then I can rewrite this to use FileCheck.
There was a problem hiding this comment.
if it works right now, and reasonably can continue to do so for the specific example being tested - I'd keep it simple/test it exactly. If there's a specific case where we can't get the whole range - maybe add a test for that and, agian, test the exact behavior with a note "this doesn't extend to the very end of the function, and that's OK or that's a known limitation, or that's a known bug (with link to the bug)"
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/180/builds/4173 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/160/builds/4175 Here is the relevant piece of the build log for the reference |
|
Looks like an explicit triple is needed for some of the tests (should probably go on all of them, but for now I'll just add it to the tests that are failing). |
SLTozer
added a commit
that referenced
this pull request
Aug 29, 2024
Several tests for the new fake use intrinsic are failing on NVPTX buildbots due to relying on behaviour for their expected triple; this commit adds that triple to each of them to prevent failures. Fixes commit 3d08ade (#86149). Example buildbot failures: https://lab.llvm.org/buildbot/#/builders/160/builds/4175 https://lab.llvm.org/buildbot/#/builders/180/builds/4173
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/95/builds/3138 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/54/builds/1778 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/143/builds/1769 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/155/builds/1901 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/17/builds/2296 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/168/builds/2715 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/23/builds/2504 Here is the relevant piece of the build log for the reference |
|
Sorry for only raising this now. |
It's not something I've thought about, but I'm happy to take a look at it and see if a refactoring patch would be worthwhile. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/76/builds/2360 Here is the relevant piece of the build log for the reference |
SLTozer
added a commit
that referenced
this pull request
Aug 29, 2024
One of the tests for the new fake use intrinsic are failing on darwin buildbots due to relying on behaviour for their expected triple; this commit adds explicit triples to the few remaining fake-use tests that didn't have them. Fixes commit 3d08ade (#86149). Buildbot failures: https://lab.llvm.org/buildbot/#/builders/23/builds/2505
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/1960 Here is the relevant piece of the build log for the reference |
Thanks! Just to clarify, I am not 100% sure that isPosition is the correct place to put this, but it seems like the best fit. If we find that isPosition doesn't work, we probably still want FAKE_USE to return false in MachineInstr::isSafeToMove() in some way or another though. isDebugInstr might also be worth considering, but I'm guessing it's not really a debug instruction? |
FAKE_USE influences code generation so as a matter of principle it should not be thought of as a debug instruction. (Those are going away anyhow, but there's a dividing line there.) |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/145/builds/1561 Here is the relevant piece of the build log for the reference |
|
We are seeing a test failure in our builders which I think is related to this patch: I am investigating further to confirm. |
Some of the tests added in this patch didn't have an explicit triple, which was necessary for the test to pass for some targets - I've already committed updates to add those triples, by the looks of it the test has started passing as of that commit: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/125/builds/1697 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/39/builds/1330 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/42/builds/900 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/146/builds/1001 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/136/builds/770 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/64/builds/820 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/98/builds/394 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/349 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch is part of a set of patches that add an
-fextend-lifetimesflag to clang, which extends the lifetimes of local variables and parameters for improved debuggability. In addition to that flag, the patch series adds a pragma to selectively disable-fextend-lifetimes, and an-fextend-this-ptrflag which functions as-fextend-lifetimesfor this pointers only. All changes and tests in these patches were written by @wolfy1961, though I will be managing these reviews and addressing any comments raised. The extend lifetimes flag is intended to eventually be set on by-Og, as discussed in the RFC here.This patch implements a new intrinsic instruction in LLVM,
llvm.fake.usein IR andFAKE_USEin MIR, that takes a single operand and has no effect other than "using" its operand, to ensure that its operand remains live until after the fake use. This patch does not emit fake uses anywhere; the next patch in this sequence causes them to be emitted from the clang frontend, such that for each variable (or this) a fake.use operand is inserted at the end of that variable's scope, using that variable's value. This patch covers everything post-frontend, which is largely just the basic plumbing for a new intrinsic/instruction, along with a few steps to preserve the fake uses through optimizations (such as moving them ahead of a tail call or translating them through SROA).