<div dir="ltr"><div>Oops - failed to hit send. Sorry about the delayed reply. ><</div><div><br></div>Hi Hal,<div><br></div><div>> Wouldn't it be better just to emit:<br>><br>> call _foo<br></div><div><br></div><div>Sounds good to me.</div><div><br></div><div>Quick background: When I originally wrote this I decided to match the non-symbolic code sequence so that I could be 100% certain not to break anyone. Reading <span style="line-height:normal"><a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_docs_StackMaps.html&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=01XFUk6pwZbXGbrmz9bKwmlQOq3fCj3igABkx5v-RWE&s=h0iRZCzDxe6ErggFqtpBMoDTMWWxUCRSAg6egdc2V7Q&e=">http://llvm.org/docs/StackMaps.html</a> though,</span><span style="line-height:normal"> I see that clients have never been allowed to assume anything about the code sequence, so this should be safe.</span></div><div><span style="color:rgb(0,0,0);font-family:'Lucida Grande','Lucida Sans Unicode',Geneva,Verdana,sans-serif;font-size:14px;line-height:21px"><br></span></div><div>Cheers,</div><div>Lang.</div><div><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Jul 11, 2015 at 9:18 PM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div>----- Original Message -----<br>
> From: "Lang Hames" <<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>><br>
> To: <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
> Sent: Wednesday, April 22, 2015 1:02:32 AM<br>
> Subject: [llvm] r235483 - [patchpoint] Add support for symbolic patchpoint targets to SelectionDAG and the<br>
><br>
> Author: lhames<br>
> Date: Wed Apr 22 01:02:31 2015<br>
> New Revision: 235483<br>
><br>
> URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D235483-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=01XFUk6pwZbXGbrmz9bKwmlQOq3fCj3igABkx5v-RWE&s=a7mD43LTtNCDmMGTalt54eHmqO7ungGY6bj89Cp5mQE&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=235483&view=rev</a><br>
> Log:<br>
> [patchpoint] Add support for symbolic patchpoint targets to<br>
> SelectionDAG and the<br>
> X86 backend.<br>
><br>
> The code generated for symbolic targets is identical to the code<br>
> generated for<br>
> constant targets, except that a relocation is emitted to fix up the<br>
> actual<br>
> target address at link-time. This allows IR and object files<br>
> containing<br>
> patchpoints to be cached across JIT-invocations where the target<br>
> address may<br>
> change.<br>
><br>
><br>
> Modified:<br>
> llvm/trunk/include/llvm/MC/MCInstBuilder.h<br>
> llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp<br>
> llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp<br>
> llvm/trunk/lib/Target/X86/X86AsmPrinter.h<br>
> llvm/trunk/lib/Target/X86/X86MCInstLower.cpp<br>
> llvm/trunk/test/CodeGen/X86/patchpoint.ll<br>
><br>
> Modified: llvm/trunk/include/llvm/MC/MCInstBuilder.h<br>
> URL:<br>
> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_include_llvm_MC_MCInstBuilder.h-3Frev-3D235483-26r1-3D235482-26r2-3D235483-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=01XFUk6pwZbXGbrmz9bKwmlQOq3fCj3igABkx5v-RWE&s=YRnRMS3y6ThTRtWRpwoDVRQyEPaaG5AZssmi50qoLdU&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/MC/MCInstBuilder.h?rev=235483&r1=235482&r2=235483&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/include/llvm/MC/MCInstBuilder.h (original)<br>
> +++ llvm/trunk/include/llvm/MC/MCInstBuilder.h Wed Apr 22 01:02:31<br>
> 2015<br>
> @@ -58,6 +58,12 @@ public:<br>
> return *this;<br>
> }<br>
><br>
> + /// \brief Add an operand.<br>
> + MCInstBuilder &addOperand(const MCOperand &Op) {<br>
> + Inst.addOperand(Op);<br>
> + return *this;<br>
> + }<br>
> +<br>
> operator MCInst&() {<br>
> return Inst;<br>
> }<br>
><br>
> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp<br>
> URL:<br>
> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_lib_CodeGen_SelectionDAG_FastISel.cpp-3Frev-3D235483-26r1-3D235482-26r2-3D235483-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=01XFUk6pwZbXGbrmz9bKwmlQOq3fCj3igABkx5v-RWE&s=cBqXwq24ai-Mozyj_FlqpBg-uq_OapMwKLye-J4LqjA&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp?rev=235483&r1=235482&r2=235483&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp (original)<br>
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp Wed Apr 22<br>
> 01:02:31 2015<br>
> @@ -711,7 +711,7 @@ bool FastISel::selectPatchpoint(const Ca<br>
> CallingConv::ID CC = I->getCallingConv();<br>
> bool IsAnyRegCC = CC == CallingConv::AnyReg;<br>
> bool HasDef = !I->getType()->isVoidTy();<br>
> - Value *Callee = I->getOperand(PatchPointOpers::TargetPos);<br>
> + Value *Callee =<br>
> I->getOperand(PatchPointOpers::TargetPos)->stripPointerCasts();<br>
><br>
> // Get the real number of arguments participating in the call<br>
> <numArgs><br>
> assert(isa<ConstantInt>(I->getOperand(PatchPointOpers::NArgPos))<br>
> &&<br>
> @@ -757,23 +757,25 @@ bool FastISel::selectPatchpoint(const Ca<br>
> cast<ConstantInt>(I->getOperand(PatchPointOpers::NBytesPos));<br>
> Ops.push_back(MachineOperand::CreateImm(NumBytes->getZExtValue()));<br>
><br>
> - // Assume that the callee is a constant address or null pointer.<br>
> - // FIXME: handle function symbols in the future.<br>
> - uint64_t CalleeAddr;<br>
> - if (const auto *C = dyn_cast<IntToPtrInst>(Callee))<br>
> - CalleeAddr =<br>
> cast<ConstantInt>(C->getOperand(0))->getZExtValue();<br>
> - else if (const auto *C = dyn_cast<ConstantExpr>(Callee)) {<br>
> - if (C->getOpcode() == Instruction::IntToPtr)<br>
> - CalleeAddr =<br>
> cast<ConstantInt>(C->getOperand(0))->getZExtValue();<br>
> - else<br>
> + // Add the call target.<br>
> + if (const auto *C = dyn_cast<IntToPtrInst>(Callee)) {<br>
> + uint64_t CalleeConstAddr =<br>
> + cast<ConstantInt>(C->getOperand(0))->getZExtValue();<br>
> + Ops.push_back(MachineOperand::CreateImm(CalleeConstAddr));<br>
> + } else if (const auto *C = dyn_cast<ConstantExpr>(Callee)) {<br>
> + if (C->getOpcode() == Instruction::IntToPtr) {<br>
> + uint64_t CalleeConstAddr =<br>
> + cast<ConstantInt>(C->getOperand(0))->getZExtValue();<br>
> + Ops.push_back(MachineOperand::CreateImm(CalleeConstAddr));<br>
> + } else<br>
> llvm_unreachable("Unsupported ConstantExpr.");<br>
> + } else if (const auto *GV = dyn_cast<GlobalValue>(Callee)) {<br>
> + Ops.push_back(MachineOperand::CreateGA(GV, 0));<br>
> } else if (isa<ConstantPointerNull>(Callee))<br>
> - CalleeAddr = 0;<br>
> + Ops.push_back(MachineOperand::CreateImm(0));<br>
> else<br>
> llvm_unreachable("Unsupported callee address.");<br>
><br>
> - Ops.push_back(MachineOperand::CreateImm(CalleeAddr));<br>
> -<br>
> // Adjust <numArgs> to account for any arguments that have been<br>
> passed on<br>
> // the stack instead.<br>
> unsigned NumCallRegArgs = IsAnyRegCC ? NumArgs :<br>
> CLI.OutRegs.size();<br>
><br>
> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp<br>
> URL:<br>
> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_lib_CodeGen_SelectionDAG_SelectionDAGBuilder.cpp-3Frev-3D235483-26r1-3D235482-26r2-3D235483-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=01XFUk6pwZbXGbrmz9bKwmlQOq3fCj3igABkx5v-RWE&s=Ym81Vt4m8ks1aUh97Kilb9DRXnDbJNr026CSMW_mMgE&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp?rev=235483&r1=235482&r2=235483&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp<br>
> (original)<br>
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp Wed<br>
> Apr 22 01:02:31 2015<br>
> @@ -7001,7 +7001,16 @@ void SelectionDAGBuilder::visitPatchpoin<br>
> CallingConv::ID CC = CS.getCallingConv();<br>
> bool IsAnyRegCC = CC == CallingConv::AnyReg;<br>
> bool HasDef = !CS->getType()->isVoidTy();<br>
> - SDValue Callee = getValue(CS->getOperand(2)); // <target><br>
> + SDValue Callee =<br>
> getValue(CS->getOperand(PatchPointOpers::TargetPos));<br>
> +<br>
> + // Handle immediate and symbolic callees.<br>
> + if (auto* ConstCallee = dyn_cast<ConstantSDNode>(Callee))<br>
> + Callee = DAG.getIntPtrConstant(ConstCallee->getZExtValue(),<br>
> + /*isTarget=*/true);<br>
> + else if (auto* SymbolicCallee =<br>
> dyn_cast<GlobalAddressSDNode>(Callee))<br>
> + Callee =<br>
> DAG.getTargetGlobalAddress(SymbolicCallee->getGlobal(),<br>
> + SDLoc(SymbolicCallee),<br>
> +<br>
> SymbolicCallee->getValueType(0));<br>
><br>
> // Get the real number of arguments participating in the call<br>
> <numArgs><br>
> SDValue NArgVal =<br>
> getValue(CS.getArgument(PatchPointOpers::NArgPos));<br>
> @@ -7041,11 +7050,8 @@ void SelectionDAGBuilder::visitPatchpoin<br>
> Ops.push_back(DAG.getTargetConstant(<br>
> cast<ConstantSDNode>(NBytesVal)->getZExtValue(),<br>
> MVT::i32));<br>
><br>
> - // Assume that the Callee is a constant address.<br>
> - // FIXME: handle function symbols in the future.<br>
> - Ops.push_back(<br>
> -<br>
> DAG.getIntPtrConstant(cast<ConstantSDNode>(Callee)->getZExtValue(),<br>
> - /*isTarget=*/true));<br>
> + // Add the callee.<br>
> + Ops.push_back(Callee);<br>
><br>
> // Adjust <numArgs> to account for any arguments that have been<br>
> passed on the<br>
> // stack instead.<br>
><br>
> Modified: llvm/trunk/lib/Target/X86/X86AsmPrinter.h<br>
> URL:<br>
> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_lib_Target_X86_X86AsmPrinter.h-3Frev-3D235483-26r1-3D235482-26r2-3D235483-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=01XFUk6pwZbXGbrmz9bKwmlQOq3fCj3igABkx5v-RWE&s=xoIJe_ws3pD9tu11BjfoJ_LPa6VrVd62WQ6tJPdnFP4&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86AsmPrinter.h?rev=235483&r1=235482&r2=235483&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Target/X86/X86AsmPrinter.h (original)<br>
> +++ llvm/trunk/lib/Target/X86/X86AsmPrinter.h Wed Apr 22 01:02:31<br>
> 2015<br>
> @@ -81,7 +81,7 @@ class LLVM_LIBRARY_VISIBILITY X86AsmPrin<br>
><br>
> void InsertStackMapShadows(MachineFunction &MF);<br>
> void LowerSTACKMAP(const MachineInstr &MI);<br>
> - void LowerPATCHPOINT(const MachineInstr &MI);<br>
> + void LowerPATCHPOINT(const MachineInstr &MI, X86MCInstLower<br>
> &MCIL);<br>
><br>
> void LowerTlsAddr(X86MCInstLower &MCInstLowering, const<br>
> MachineInstr &MI);<br>
><br>
><br>
> Modified: llvm/trunk/lib/Target/X86/X86MCInstLower.cpp<br>
> URL:<br>
> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_lib_Target_X86_X86MCInstLower.cpp-3Frev-3D235483-26r1-3D235482-26r2-3D235483-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=01XFUk6pwZbXGbrmz9bKwmlQOq3fCj3igABkx5v-RWE&s=qRie156syBtVn1CJDz2akwGLr-Oldhil0HZR0Pw73Y8&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86MCInstLower.cpp?rev=235483&r1=235482&r2=235483&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Target/X86/X86MCInstLower.cpp (original)<br>
> +++ llvm/trunk/lib/Target/X86/X86MCInstLower.cpp Wed Apr 22 01:02:31<br>
> 2015<br>
> @@ -867,7 +867,8 @@ void X86AsmPrinter::LowerSTACKMAP(const<br>
><br>
> // Lower a patchpoint of the form:<br>
> // [<def>], <id>, <numBytes>, <target>, <numArgs>, <cc>, ...<br>
> -void X86AsmPrinter::LowerPATCHPOINT(const MachineInstr &MI) {<br>
> +void X86AsmPrinter::LowerPATCHPOINT(const MachineInstr &MI,<br>
> + X86MCInstLower &MCIL) {<br>
> assert(Subtarget->is64Bit() && "Patchpoint currently only supports<br>
> X86-64");<br>
><br>
> SMShadowTracker.emitShadowPadding(OutStreamer,<br>
> getSubtargetInfo());<br>
> @@ -877,8 +878,29 @@ void X86AsmPrinter::LowerPATCHPOINT(cons<br>
> PatchPointOpers opers(&MI);<br>
> unsigned ScratchIdx = opers.getNextScratchIdx();<br>
> unsigned EncodedBytes = 0;<br>
> - int64_t CallTarget =<br>
> opers.getMetaOper(PatchPointOpers::TargetPos).getImm();<br>
> - if (CallTarget) {<br>
> + const MachineOperand &CalleeMO =<br>
> + opers.getMetaOper(PatchPointOpers::TargetPos);<br>
> +<br>
> + // Check for null target. If target is non-null (i.e. is non-zero<br>
> or is<br>
> + // symbolic) then emit a call.<br>
> + if (!(CalleeMO.isImm() && !CalleeMO.getImm())) {<br>
> + MCOperand CalleeMCOp;<br>
> + switch (CalleeMO.getType()) {<br>
> + default:<br>
> + /// FIXME: Add a verifier check for bad callee types.<br>
> + llvm_unreachable("Unrecognized callee operand type.");<br>
> + case MachineOperand::MO_Immediate:<br>
> + if (CalleeMO.getImm())<br>
> + CalleeMCOp = MCOperand::CreateImm(CalleeMO.getImm());<br>
> + break;<br>
> + case MachineOperand::MO_ExternalSymbol:<br>
> + case MachineOperand::MO_GlobalAddress:<br>
> + CalleeMCOp =<br>
> + MCIL.LowerSymbolOperand(CalleeMO,<br>
> +<br>
> MCIL.GetSymbolFromOperand(CalleeMO));<br>
> + break;<br>
> + }<br>
> +<br>
> // Emit MOV to materialize the target address and the CALL to<br>
> target.<br>
> // This is encoded with 12-13 bytes, depending on which register<br>
> is used.<br>
> unsigned ScratchReg = MI.getOperand(ScratchIdx).getReg();<br>
> @@ -886,10 +908,12 @@ void X86AsmPrinter::LowerPATCHPOINT(cons<br>
> EncodedBytes = 13;<br>
> else<br>
> EncodedBytes = 12;<br>
> -<br>
> EmitAndCountInstruction(MCInstBuilder(X86::MOV64ri).addReg(ScratchReg)<br>
> -<br>
> .addImm(CallTarget));<br>
> +<br>
> + EmitAndCountInstruction(<br>
> +<br>
> MCInstBuilder(X86::MOV64ri).addReg(ScratchReg).addOperand(CalleeMCOp));<br>
> EmitAndCountInstruction(MCInstBuilder(X86::CALL64r).addReg(ScratchReg));<br>
> }<br>
> +<br>
> // Emit padding.<br>
> unsigned NumBytes =<br>
> opers.getMetaOper(PatchPointOpers::NBytesPos).getImm();<br>
> assert(NumBytes >= EncodedBytes &&<br>
> @@ -1091,7 +1115,7 @@ void X86AsmPrinter::EmitInstruction(cons<br>
> return LowerSTACKMAP(*MI);<br>
><br>
> case TargetOpcode::PATCHPOINT:<br>
> - return LowerPATCHPOINT(*MI);<br>
> + return LowerPATCHPOINT(*MI, MCInstLowering);<br>
><br>
> case X86::MORESTACK_RET:<br>
> EmitAndCountInstruction(MCInstBuilder(getRetOpcode(*Subtarget)));<br>
><br>
> Modified: llvm/trunk/test/CodeGen/X86/patchpoint.ll<br>
> URL:<br>
> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_test_CodeGen_X86_patchpoint.ll-3Frev-3D235483-26r1-3D235482-26r2-3D235483-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=01XFUk6pwZbXGbrmz9bKwmlQOq3fCj3igABkx5v-RWE&s=0JzJVZh8w65zT41n77wAe2hxnnOr7uC2vwwvQ2zICAw&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/patchpoint.ll?rev=235483&r1=235482&r2=235483&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/test/CodeGen/X86/patchpoint.ll (original)<br>
> +++ llvm/trunk/test/CodeGen/X86/patchpoint.ll Wed Apr 22 01:02:31<br>
> 2015<br>
> @@ -21,6 +21,22 @@ entry:<br>
> ret i64 %result<br>
> }<br>
><br>
> +; Trivial symbolic patchpoint codegen.<br>
> +;<br>
> +<br>
> +declare i64 @foo(i64 %p1, i64 %p2)<br>
> +define i64 @trivial_symbolic_patchpoint_codegen(i64 %p1, i64 %p2) {<br>
> +entry:<br>
> +; CHECK-LABEL: trivial_symbolic_patchpoint_codegen:<br>
> +; CHECK: movabsq $_foo, %r11<br>
> +; CHECK-NEXT: callq *%r11<br>
<br>
</div></div>Wouldn't it be better just to emit:<br>
<br>
callq _foo<br>
<br>
instead of the move and then the indirect call?<br>
<br>
-Hal<br>
<div><div><br>
> +; CHECK-NEXT: xchgw %ax, %ax<br>
> +; CHECK: retq<br>
> + %result = tail call i64 (i64, i32, i8*, i32, ...)<br>
> @llvm.experimental.patchpoint.i64(i64 9, i32 15, i8* bitcast (i64<br>
> (i64, i64)* @foo to i8*), i32 2, i64 %p1, i64 %p2)<br>
> + ret i64 %result<br>
> +}<br>
> +<br>
> +<br>
> ; Caller frame metadata with stackmaps. This should not be optimized<br>
> ; as a leaf function.<br>
> ;<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
<br>
</div></div><span><font color="#888888">--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</font></span></blockquote></div><br></div></div>