-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[IR] llvm.reloc.none intrinsic for no-op symbol references #147427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@llvm/pr-subscribers-llvm-globalisel @llvm/pr-subscribers-llvm-ir Author: Daniel Thornburgh (mysterymath) ChangesThis intrinsic emits a BFD_RELOC_NONE relocation at the point of call, which allows optimizations and languages to explicitly pull in symbols from static libraries without there being any code or data that has an effectual relocation against such a symbol. See issue #146159 for context. Full diff: https://github.com/llvm/llvm-project/pull/147427.diff 10 Files Affected:
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index cc72a37f68599..187182e5357e7 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -30439,6 +30439,38 @@ 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.
+.. _llvm_reloc_none:
+
+'``llvm.reloc.none``' Intrinsic
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Syntax:
+"""""""
+
+::
+
+ declare void @llvm.reloc.none(ptrty %ptr)
+
+Overview:
+"""""""""
+
+The ``llvm.reloc.none`` intrinsic emits a no-op relocation against a given
+operand symbol. This can bring the symbol
+definition into the link without emitting any code or data to the binary for
+that purpose.
+
+Arguments:
+""""""""""
+
+The ``llvm.fake.use`` intrinsic takes one argument, which may be any global
+value.
+
+Semantics:
+""""""""""
+
+This intrinsic emits a no-op relocation at the location of the intrinsic call
+for the symbol that corresponds to the global value argument.
+
Stack Map Intrinsics
--------------------
diff --git a/llvm/include/llvm/CodeGen/ISDOpcodes.h b/llvm/include/llvm/CodeGen/ISDOpcodes.h
index 465e4a0a9d0d8..9c36f9c4fe525 100644
--- a/llvm/include/llvm/CodeGen/ISDOpcodes.h
+++ b/llvm/include/llvm/CodeGen/ISDOpcodes.h
@@ -1531,6 +1531,9 @@ enum NodeType {
#define BEGIN_REGISTER_VP_SDNODE(VPSDID, ...) VPSDID,
#include "llvm/IR/VPIntrinsics.def"
+ // Issue a no-op relocation against a given symbol at the current location.
+ RELOC_NONE,
+
// The `llvm.experimental.convergence.*` intrinsics.
CONVERGENCECTRL_ANCHOR,
CONVERGENCECTRL_ENTRY,
diff --git a/llvm/include/llvm/CodeGen/SelectionDAGISel.h b/llvm/include/llvm/CodeGen/SelectionDAGISel.h
index 375d2f31e2835..3a399c39c496f 100644
--- a/llvm/include/llvm/CodeGen/SelectionDAGISel.h
+++ b/llvm/include/llvm/CodeGen/SelectionDAGISel.h
@@ -473,6 +473,7 @@ class SelectionDAGISel {
void Select_WRITE_REGISTER(SDNode *Op);
void Select_UNDEF(SDNode *N);
void Select_FAKE_USE(SDNode *N);
+ void Select_RELOC_NONE(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 7add4a27ce9e9..131bc56c32362 100644
--- a/llvm/include/llvm/IR/Intrinsics.td
+++ b/llvm/include/llvm/IR/Intrinsics.td
@@ -1917,6 +1917,9 @@ def int_threadlocal_address : DefaultAttrsIntrinsic<[llvm_anyptr_ty], [LLVMMatch
def int_stepvector : DefaultAttrsIntrinsic<[llvm_anyvector_ty],
[], [IntrNoMem]>;
+def int_reloc_none : DefaultAttrsIntrinsic<[], [llvm_ptr_ty],
+ [IntrHasSideEffects, IntrInaccessibleMemOnly, IntrWillReturn]>;
+
//===---------------- Vector Predication Intrinsics --------------===//
// Memory Intrinsics
def int_vp_store : DefaultAttrsIntrinsic<[],
diff --git a/llvm/include/llvm/Support/TargetOpcodes.def b/llvm/include/llvm/Support/TargetOpcodes.def
index 92fd60e03112a..5fd0b2a06188e 100644
--- a/llvm/include/llvm/Support/TargetOpcodes.def
+++ b/llvm/include/llvm/Support/TargetOpcodes.def
@@ -233,6 +233,9 @@ HANDLE_TARGET_OPCODE(MEMBARRIER)
// using.
HANDLE_TARGET_OPCODE(JUMP_TABLE_DEBUG_INFO)
+// Issue a no-op relocation against a given symbol at the current location.
+HANDLE_TARGET_OPCODE(RELOC_NONE)
+
HANDLE_TARGET_OPCODE(CONVERGENCECTRL_ENTRY)
HANDLE_TARGET_OPCODE(CONVERGENCECTRL_ANCHOR)
HANDLE_TARGET_OPCODE(CONVERGENCECTRL_LOOP)
diff --git a/llvm/include/llvm/Target/Target.td b/llvm/include/llvm/Target/Target.td
index ce9a2b2751968..fca69107c8b55 100644
--- a/llvm/include/llvm/Target/Target.td
+++ b/llvm/include/llvm/Target/Target.td
@@ -1532,6 +1532,11 @@ def JUMP_TABLE_DEBUG_INFO : StandardPseudoInstruction {
let Size = 0;
let isMeta = true;
}
+def RELOC_NONE : StandardPseudoInstruction {
+ let OutOperandList = (outs);
+ let InOperandList = (ins unknown:$symbol);
+ let hasSideEffects = true;
+}
let hasSideEffects = false, isMeta = true, isConvergent = true in {
def CONVERGENCECTRL_ANCHOR : StandardPseudoInstruction {
diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
index a2c3b50b24670..e6206e6cbb6ae 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -1930,6 +1930,20 @@ void AsmPrinter::emitFunctionBody() {
// This is only used to influence register allocation behavior, no
// actual initialization is needed.
break;
+ case TargetOpcode::RELOC_NONE: {
+ // Generate a temporary label for the current PC.
+ MCSymbol *Sym = OutContext.createTempSymbol("reloc_none");
+ OutStreamer->emitLabel(Sym);
+ const MCExpr *Dot = MCSymbolRefExpr::create(Sym, OutContext);
+
+ assert(MI.getNumOperands() == 1 &&
+ "RELOC_NONE can only have one operand");
+ const MCExpr *Value = MCSymbolRefExpr::create(
+ getSymbol(MI.getOperand(0).getGlobal()), OutContext);
+ OutStreamer->emitRelocDirective(*Dot, "BFD_RELOC_NONE", Value, SMLoc(),
+ *STI);
+ break;
+ }
default:
emitInstruction(&MI);
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index c01f1e7928477..bcfbca582d699 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -7757,6 +7757,19 @@ void SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I,
return;
}
+ case Intrinsic::reloc_none: {
+ SDValue V = getValue(I.getArgOperand(0));
+ auto *GA = dyn_cast<GlobalAddressSDNode>(V);
+ if (!GA)
+ report_fatal_error("llvm.reloc.none operand must be a GlobalValue");
+ SDValue Ops[2];
+ Ops[0] = getRoot();
+ Ops[1] = DAG.getTargetGlobalAddress(GA->getGlobal(), sdl, V.getValueType(),
+ GA->getOffset());
+ DAG.setRoot(DAG.getNode(ISD::RELOC_NONE, sdl, MVT::Other, Ops));
+ return;
+ }
+
case Intrinsic::eh_exceptionpointer:
case Intrinsic::eh_exceptioncode: {
// Get the exception pointer vreg, copy from it, and resize it to fit.
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
index 7fc15581c17e4..5296913e718c2 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
@@ -471,6 +471,8 @@ std::string SDNode::getOperationName(const SelectionDAG *G) const {
case ISD::LIFETIME_END: return "lifetime.end";
case ISD::FAKE_USE:
return "fake_use";
+ case ISD::RELOC_NONE:
+ return "reloc_none";
case ISD::PSEUDO_PROBE:
return "pseudoprobe";
case ISD::GC_TRANSITION_START: return "gc_transition.start";
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
index 4b98d87fcc63b..ccc583c6a2b8d 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
@@ -2498,6 +2498,11 @@ void SelectionDAGISel::Select_FAKE_USE(SDNode *N) {
N->getOperand(1), N->getOperand(0));
}
+void SelectionDAGISel::Select_RELOC_NONE(SDNode *N) {
+ CurDAG->SelectNodeTo(N, TargetOpcode::RELOC_NONE, N->getValueType(0),
+ N->getOperand(1), N->getOperand(0));
+}
+
void SelectionDAGISel::Select_FREEZE(SDNode *N) {
// TODO: We don't have FREEZE pseudo-instruction in MachineInstr-level now.
// If FREEZE instruction is added later, the code below must be changed as
@@ -3273,6 +3278,9 @@ void SelectionDAGISel::SelectCodeCommon(SDNode *NodeToMatch,
case ISD::FAKE_USE:
Select_FAKE_USE(NodeToMatch);
return;
+ case ISD::RELOC_NONE:
+ Select_RELOC_NONE(NodeToMatch);
+ return;
case ISD::FREEZE:
Select_FREEZE(NodeToMatch);
return;
|
|
Sending this out as a draft to obtain some early feedback about the direction of the implemenation of the Modular Printf RFC. Next PR in chain: #147429 |
| auto *M = const_cast<Module *>(I.getModule()); | ||
| auto *RelocSymbol = cast<GlobalVariable>( | ||
| M->getOrInsertGlobal(SymbolName, StructType::create(M->getContext()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot modify the IR here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because this occurs in a MachineFunctionPass right? In that case, I got bamboozled! I had unceremoniously copy-pasted this from the lowering of Intrinsic::amdgcn_reloc_constant in SIISelLowering.cpp, which also runs in a MachineFunctionPass. I'll admit that this completely slipped past me, although I was curious about the const_cast.
AFAIK, converting an arbitrary symbol name metadata string to a GlobalValue reference instrinsically mutates the module. So, if we wanted to fix this, we could either go back to having this be a symbol reference to a pre-existing global value, or we could forward the metadata string to RELOC_NONE as a string (odd in the backend; unsure if this is even possible?) to be lowered to a symbol reference at assembly time. My preference would definitely be the former, since it has fewer unknowns.
Whichever we choose, also I'd think either way we should give amdgcn_reloc_constant the same treatment, unless it's not possible to do so for backwards compatibility reasons.
@arsenm Do you have preferences here?
EDIT: There is Metadata and MCSymbol support in MachineOperand, but it's tagged in the enum as "for debug info". We could probably use this for RELOC_NONE's argument, but maybe that would cause problems? (Or be "weird"?) Unsure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lacking more specific feedback, I rolled back to using a GlobalValue reference; that seems the most straigtforward way to resolve this. PTAL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need a GlobalValue reference to emit the symbol name in the end. You just need to encode this as an ExternalSymbol operand instead. The comment that it's "for debug info" doesn't really matter
c89242c to
657f659
Compare
|
Alright, I've filled out tests and added lowering to GlobalISel also. This should be formally ready for review (although much has already occurred). |
341dd6a to
b76a6c3
Compare
|
Friendly ping; is there anything left to do on this one? |
b76a6c3 to
bad31c7
Compare
llvm/include/llvm/IR/Intrinsics.td
Outdated
| [], [IntrNoMem]>; | ||
|
|
||
| def int_reloc_none : DefaultAttrsIntrinsic<[], [llvm_ptr_ty], | ||
| [IntrHasSideEffects, IntrInaccessibleMemOnly, IntrWillReturn]>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why define IntrHasSideEffects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New uses of llvm_ptr_ty should not be introduced. Only llvm_anyptr_ty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why define IntrHasSideEffects?
Sorry, cargo cult copying from fake_use. After skimming the doc comments, I removed everything and made it IntrNoMem. This doesn't appear to cause the intrinsic to be DCEd, which was what I was worried about. I've added a test for DCE avoidance, too.
New uses of llvm_ptr_ty should not be introduced. Only llvm_anyptr_ty
Ack; it's metadata now, so this no longer applies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why define IntrHasSideEffects?
Actually, it appears that the correct params were InstrNoMem and InstrHasSideEffects to prevent DCE. I discovered in the follow up patch that opt will DCE the intrinsic otherwise; accordingly, the fact that it isn't DCEd is tested there.
|
I'm happy to add this intrinsic, and the object file format code looks good to me. Do @arsenm and @nikic have comments? Note: If a function A with llvm.reloc.none is inlined into other functions, the section referenced by llvm.reloc.none becomes harder for the linker to garbage collect. (If a global object
Case 2 might not be desired. Using a relocation will not have the problem.) |
| SDValue Ops[2]; | ||
| Ops[0] = getRoot(); | ||
| Ops[1] = DAG.getTargetGlobalAddress(GA->getGlobal(), sdl, V.getValueType(), | ||
| GA->getOffset()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| SDValue Ops[2]; | |
| Ops[0] = getRoot(); | |
| Ops[1] = DAG.getTargetGlobalAddress(GA->getGlobal(), sdl, V.getValueType(), | |
| GA->getOffset()); | |
| SDValue Ops[2] = { | |
| getRoot(), | |
| DAG.getTargetGlobalAddress(GA->getGlobal(), sdl, V.getValueType(), | |
| GA->getOffset() }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks; done.
llvm/include/llvm/IR/Intrinsics.td
Outdated
| [], [IntrNoMem]>; | ||
|
|
||
| def int_reloc_none : DefaultAttrsIntrinsic<[], [llvm_ptr_ty], | ||
| [IntrHasSideEffects, IntrInaccessibleMemOnly, IntrWillReturn]>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New uses of llvm_ptr_ty should not be introduced. Only llvm_anyptr_ty
| auto *M = const_cast<Module *>(I.getModule()); | ||
| auto *RelocSymbol = cast<GlobalVariable>( | ||
| M->getOrInsertGlobal(SymbolName, StructType::create(M->getContext()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need a GlobalValue reference to emit the symbol name in the end. You just need to encode this as an ExternalSymbol operand instead. The comment that it's "for debug info" doesn't really matter
bad31c7 to
8c30818
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally got back around to this one. I've modified the backend pseudo to use an ExternalSymbol instead and the IR intrinsic to take metadata once gain; this ended up much cleaner, as well as fixing the invariant violation. I had originally tried this, but I had thought from the interface that ExternalSymbol could only take a constant string or something interned and null terminated. Thankfully, metadata strings are interned and null terminated, although it was surprisingly difficult to figure that out.
That should address the outstanding comments; PTAL and let me know if there's anything else.
llvm/include/llvm/IR/Intrinsics.td
Outdated
| [], [IntrNoMem]>; | ||
|
|
||
| def int_reloc_none : DefaultAttrsIntrinsic<[], [llvm_ptr_ty], | ||
| [IntrHasSideEffects, IntrInaccessibleMemOnly, IntrWillReturn]>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why define IntrHasSideEffects?
Sorry, cargo cult copying from fake_use. After skimming the doc comments, I removed everything and made it IntrNoMem. This doesn't appear to cause the intrinsic to be DCEd, which was what I was worried about. I've added a test for DCE avoidance, too.
New uses of llvm_ptr_ty should not be introduced. Only llvm_anyptr_ty
Ack; it's metadata now, so this no longer applies.
| SDValue Ops[2]; | ||
| Ops[0] = getRoot(); | ||
| Ops[1] = DAG.getTargetGlobalAddress(GA->getGlobal(), sdl, V.getValueType(), | ||
| GA->getOffset()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks; done.
This intrinsic emits a BFD_RELOC_NONE relocation at the point of call, which allows optimizations and languages to explicitly pull in symbols from static libraries without there being any code or data that has an effectual relocation against such a symbol. See issue #146159 for context.
This reverts commit b01bf98. However, this time, the metadata string is passed as a string directly to the backend. This prevents the affected MachineFunction passes from incorrectly modifying the IR.
8c30818 to
822f240
Compare
This intrinsic emits a BFD_RELOC_NONE relocation at the point of call, which allows optimizations and languages to explicitly pull in symbols from static libraries without there being any code or data that has an effectual relocation against such a symbol.
See issue #146159 for context.