[ISEL][X86] Tracking of registers that forward call arguments

[djtodoro]

Summary

While lowering calls collect info about registers that forward arguments into following function frame. We store such info into MachineFunction of the call.
This is used very late when dumping DWARF info about call site parameters.

NOTE: Other architectures should follow similar procedure buy collecting call argument forwarding register in theirs LowerCallspecialization. Once the call site info is collected it should be bound to SDNode that represents the call instruction. Backend support should keep track about updating MachineInstr call instructions. For more info about backend call site support see https://reviews.llvm.org/D61062.

Authors: @asowda, @NikolaPrica, @djtodoro, @ivanbaev

[aprantl]

Looking through the file list, this seems to be missing FastISel and GlobalIsel support?

include/llvm/CodeGen/MIRYamlMapping.h

340 ↗

(On Diff #195196)

This could use some Doxygen comments :-)

[bjope]

Looks generally good. But...

I think the subject and summary of this patch is a little bit misleading. It talks about ISEL and lowering of calls, but that only seems to be a small part of it.

Afaict this patch only implements the "full" call lowering support for X86, right? What should happen for other targets when using -emit-call-site-info (afaict SelectionDAG still would generate some callsite information)?
Should probably elaborate a little bit about only supporting X86 and what other targets would need to do.

A substantial part of this patch is touching Codegen in a more general way to support storing the callsite info in the MachineFunction, and serializing that information in the MIR format. Not really related to ISEL IMHO :-)
Should perhaps MIRLangReg.rst be updated in this patch to describe the new additions (or is that covered by another patch in the stack)?.

The patch is also updating passes like InlineSpiller, PeepholeOptimizer and MachineOutliner (this is not about ISEL either) to support keeping the callsite info valid and up-to-date through the backend.
I guess this is putting some new requirements on all backend passes. It would be good if the summary (commit msg) would explain a little bit more about the implications with the solution (such as when MF.updateCallSiteInfo(...); should be used). If I for example want to use this functionality in my OOT target such information is extra helpful, as I need to make all such patches myself. But such information could also be useful for in-tree development, e.g. for reviewers to understand if there are more places to update that you have missed (or for other patches that would need to rebase onto this patch later).

lib/CodeGen/TargetInstrInfo.cpp

138 ↗

(On Diff #195196)

Not sure if it is worth the complexity to mix the update of call site info and erasing in the same loop here? Always tricky to remove elements while iterating over the same data structure (even if I think you got it correct here).

test/CodeGen/X86/call-site-info-output.ll

8

Afaict offset: 14 just matches what we currently get (so it is not important that we get 14 here). We should check for offset: {{[0-9]+}} instead, to make the testcase less sensitive to future unrelated changes in lowering, right?

12

This verifies that the parser accepts the new syntax. But not really that it does anything useful with it (it could just discard it and this test would still pass). Maybe that is supposed to be tested elsewhere? Otherwise one idea could be to stop before expand-isel-pseudos in the RUN above, then run expand-isel-pseudos and check that we still get the same callSites info in the output, and then start-after expand-isel-pseudos for the last part of the testing.

[NikolaPrica]

@bjope @aprantl Thank you for your comments!

In D60715#1469515, @aprantl wrote:

Looking through the file list, this seems to be missing FastISel and GlobalIsel support?

Yes. We taught that it might go as a separate patch? Or should we add it all together?

Firstly this patch was in two parts but for the first review we left it as this in order to see big picture. We will separate it on ISEL part and backend handling part for sure!

Should probably elaborate a little bit about only supporting X86 and what other targets would need to do.

We will do that.

A substantial part of this patch is touching Codegen in a more general way to support storing the callsite info in the MachineFunction, and serializing that information in the MIR format. Not really related to ISEL IMHO :-)
Should perhaps MIRLangReg.rst be updated in this patch to describe the new additions (or is that covered by another patch in the stack)?.

You are right about this but since we produce such data at ISEL phase we saw it as whole. We provide it like that in order to be able to provide tests that we actually changed something in MachineFunction. Like this we can add test that MIR could be parsed by reading .mir produced from test file. But we can separate it on general part that can read given MIR format and ISEL part for X86 that would test writing MIR? Do you think that it will look better like that?

include/llvm/CodeGen/MIRYamlMapping.h

340 ↗

(On Diff #195196)

Oh sorry about that. Will add those!

lib/CodeGen/TargetInstrInfo.cpp

138 ↗

(On Diff #195196)

This is good question since we have some doubts about this. All updateCallSiteInfo(MI) are used to inform MachineFunction.CallSitesInfo that referenced call instruction got deleted and that it needs update. Now such update can be moved to MachineFunction::DeleteMachineInstr to preserve valid state of CallSiteInfo but we are not quite sure about it since it is frequently used function.
There we left assertion (it should be warped by NDEBUG or removed) that is tested for X86 but will definitely be useful for adding call site info support for other architectures. That assertion brought us to fix this function.
General question is should we move preservation of call site information state to MachineFunction::DeleteMachineInstr or just use assertion in it to point us where should we insert updateCallSiteInfo(MI); ?

test/CodeGen/X86/call-site-info-output.ll

12

Yes, you are right. This only verifies new syntax. Usage of this information is tested in the latest patch. Here we test output for call-site. With new RUN we can test parsing of new syntax and by so there will be no need to test "offset" since parser will verify that information. I will add this as comment for the test.

[djtodoro]

-Rebase

[aprantl]

include/llvm/CodeGen/MachineFunction.h

381 ↗

(On Diff #196054)

How about using a struct with field names instead of a std::pair?

955 ↗

(On Diff #196054)

/p -> \p

include/llvm/CodeGen/SelectionDAG.h

1634

This function appears to reimplement the operator[] of DenseMap?
I guess it doesn't insert the value...

lib/CodeGen/MIRPrinter.cpp

476 ↗

(On Diff #196054)

Can we replace this bodyless loop with something from std::algorithm?

lib/CodeGen/MachineFunction.cpp

365 ↗

(On Diff #196054)

assert (!MI->isCall(MachineInstr::IgnoreBundle) || ...)

lib/CodeGen/MachineVerifier.cpp

2150 ↗

(On Diff #196054)

typo

lib/CodeGen/TargetInstrInfo.cpp

140 ↗

(On Diff #196054)

clang-format

145 ↗

(On Diff #196054)

I'd prefer writing two loops (or even better two std::algorithms) for updating and erasing and let the optimizer merge them.

lib/CodeGen/XRayInstrumentation.cpp

114 ↗

(On Diff #196054)

So how do we make sure that people will call this in their new passes form here on? Can we verify it?

[NikolaPrica]

Thanks for the comments! I've addressed most of them. This patch will be split on 3 parts once we decide how to address comment about updating call site in TargetInstrInfo.

lib/CodeGen/TargetInstrInfo.cpp

145 ↗

(On Diff #196054)

I've tried with std::remove_if and llvm::remove_if but they require deleted MachineInstr copy assignment operator. So I came up with two solutions:

Solution 1

// Update call site info and remove all the dead instructions
 // from the end of MBB.
 while (Tail != MBB->end()) {
   auto MI = Tail++;
   if (MI->isCall())
     MBB->getParent()->updateCallSiteInfo(&*MI);
   MBB->erase(MI);
 }

Or

Solution 2

// Update call site info.
 for (auto It = Tail; It != MBB->end(); ++It)
   if (It->isCall())
     MBB->getParent()->updateCallSiteInfo(&*It)

 // Remove all the dead instructions from the end of MBB.
 for (auto It = Tail; It != MBB->end();) {
   auto MI = It++;
   MBB->erase(MI);
 }

@aprantl First example seams better than existing one. I'm wondering whether second with two loops is what you aimed to?

[bjope]

lib/CodeGen/TargetInstrInfo.cpp

145 ↗

(On Diff #196054)

The second loop in "solution 2" can be reduced to MBB->erase(Tail, MBB->end()); (that is what it used to look like). No need to complicate that part ;-)

[bjope]

lib/CodeGen/MIRPrinter.cpp

490 ↗

(On Diff #196054)

From CodingStandards.rst:

As a rule of thumb, always make sure to use llvm::sort instead of std::sort.

[NikolaPrica]

lib/CodeGen/MIRPrinter.cpp

490 ↗

(On Diff #196054)

Thanks for pointing that!

lib/CodeGen/TargetInstrInfo.cpp

145 ↗

(On Diff #196054)

MBB->erase(Tail, MBB->end()); recursively deletes elements. I taught that Adrian aimed to usage of two loops because optimizer will be able to merge them. Not sure whether it will be able to merge loop and recursive deletion call.

[djtodoro]

-Split up of introduction, production and handling of call site info

[lebedev.ri]

include/llvm/CodeGen/SelectionDAG.h

1633

The function is marked as const, yet this is a std::move().
Do you mean to only get the CallSiteInfo once?

[NikolaPrica]

include/llvm/CodeGen/SelectionDAG.h

1633

Too bad I got no warning on that. Thanks for pointing that!

Do you mean to only get the `CallSiteInfo` once?

Yes. For now it is used only in that way.

[djtodoro]

-Remove const from getSDCallSiteInfo()

[aprantl]

LGTM from my high-level point of view, but please make sure to address everybody else's feedback, too.

[djtodoro]

-Rebase

[bjope]

FYI: I have no further comments.

[aprantl]

test/CodeGen/X86/call-site-info-output.ll

13

Is there *anything* we can check in the output here? Otherwise this will bitrot quickly.

[djtodoro]

-Verify that call site info is the same after loading and running a pass over it.

[djtodoro]

-Rebase

[djtodoro]

-Follow the new order of fields inside the ArgRegPair struct (the struct is now better packed))

include/llvm/CodeGen/SelectionDAG.h

class SelectionDAG {
   ArrayRecycler<SDUse> OperandRecycler;
 
   /// Pool allocation for misc. objects that are created once per SelectionDAG.
   BumpPtrAllocator Allocator;
 
   /// Tracks dbg_value and dbg_label information through SDISel.
   SDDbgInfo *DbgInfo;
 
+  using CallSiteInfo = MachineFunction::CallSiteInfo;
+  using CallSiteInfoImpl = MachineFunction::CallSiteInfoImpl;
+  DenseMap<const SDNode *, CallSiteInfo> SDCallSiteInfo;
+
   uint16_t NextPersistentId = 0;
 
 public:
   /// Clients of various APIs that cause global effects on
   /// the DAG can optionally implement this interface.  This allows the clients
   /// to handle the various sorts of updates that happen.
   ///
   /// A DAGUpdateListener automatically registers itself with DAG when it is
public:
 
   /// \returns true if \p N is any kind of constant or build_vector of
   /// constants, int or float. If a vector, it may not necessarily be a splat.
   inline bool isConstantValueOfAnyType(SDValue N) {
     return isConstantIntBuildVectorOrConstantInt(N) ||
            isConstantFPBuildVectorOrConstantFP(N);
   }
 
+  void addCallSiteInfo(const SDNode *CallNode, CallSiteInfoImpl &&CallInfo) {
+    SDCallSiteInfo[CallNode] = std::move(CallInfo);
+  }
+
+  CallSiteInfo getSDCallSiteInfo(const SDNode *CallNode) {
+    auto I = SDCallSiteInfo.find(CallNode);
+    if (I != SDCallSiteInfo.end())
+      return std::move(I->second);
+    return CallSiteInfo();
+  }
+
 private:
   void InsertNode(SDNode *N);
   bool RemoveNodeFromCSEMaps(SDNode *N);
   void AddModifiedNodeToCSEMaps(SDNode *N);
   SDNode *FindModifiedNodeSlot(SDNode *N, SDValue Op, void *&InsertPos);
   SDNode *FindModifiedNodeSlot(SDNode *N, SDValue Op1, SDValue Op2,
                                void *&InsertPos);
   SDNode *FindModifiedNodeSlot(SDNode *N, ArrayRef<SDValue> Ops,

lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp

auto EmitNode =
     MachineBasicBlock::iterator Before = GetPrevInsn(Emitter.getInsertPos());
     Emitter.EmitNode(Node, IsClone, IsCloned, VRBaseMap);
     MachineBasicBlock::iterator After = GetPrevInsn(Emitter.getInsertPos());
 
     // If the iterator did not change, no instructions were inserted.
     if (Before == After)
       return nullptr;
 
+    MachineInstr *MI;
     if (Before == BB->end()) {
       // There were no prior instructions; the new ones must start at the
       // beginning of the block.
-      return &Emitter.getBlock()->instr_front();
+      MI = &Emitter.getBlock()->instr_front();
     } else {
       // Return first instruction after the pre-existing instructions.
-      return &*std::next(Before);
+      MI = &*std::next(Before);
     }
+
+    if (MI->isCall() && DAG->getTarget().Options.EnableDebugEntryValues)
+      MF.addCallArgsForwardingRegs(MI, DAG->getSDCallSiteInfo(Node));
+
+    return MI;
   };
 
   // If this is the first BB, emit byval parameter dbg_value's.
   if (HasDbg && BB->getParent()->begin() == MachineFunction::iterator(BB)) {
     SDDbgInfo::DbgIterator PDI = DAG->ByvalParmDbgBegin();
     SDDbgInfo::DbgIterator PDE = DAG->ByvalParmDbgEnd();
     for (; PDI != PDE; ++PDI) {
       MachineInstr *DbgMI= Emitter.EmitDbgValue(*PDI, VRBaseMap);

lib/Target/X86/X86ISelLowering.cpp

X86TargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
   bool HasNCSR = (CI && CI->hasFnAttr("no_caller_saved_registers")) ||
                  (Fn && Fn->hasFnAttribute("no_caller_saved_registers"));
   const auto *II = dyn_cast_or_null<InvokeInst>(CLI.CS.getInstruction());
   bool HasNoCfCheck =
       (CI && CI->doesNoCfCheck()) || (II && II->doesNoCfCheck());
   const Module *M = MF.getMMI().getModule();
   Metadata *IsCFProtectionSupported = M->getModuleFlag("cf-protection-branch");
 
+  MachineFunction::CallSiteInfo CSInfo;
+
   if (CallConv == CallingConv::X86_INTR)
     report_fatal_error("X86 interrupts may not be called directly");
 
   if (Attr.getValueAsString() == "true")
     isTailCall = false;
 
   if (Subtarget.isPICStyleGOT() &&
       !MF.getTarget().Options.GuaranteedTailCallOpt) {
for (unsigned I = 0, OutIndex = 0, E = ArgLocs.size(); I != E;
     if (VA.needsCustom()) {
       assert(VA.getValVT() == MVT::v64i1 &&
              "Currently the only custom case is when we split v64i1 to 2 regs");
       // Split v64i1 value into two registers
       Passv64i1ArgInRegs(dl, DAG, Chain, Arg, RegsToPass, VA, ArgLocs[++I],
                          Subtarget);
     } else if (VA.isRegLoc()) {
       RegsToPass.push_back(std::make_pair(VA.getLocReg(), Arg));
+      const TargetOptions &Options = DAG.getTarget().Options;
+      if (Options.EnableParamEntryValues)
+        CSInfo.emplace_back(I, VA.getLocReg());
       if (isVarArg && IsWin64) {
         // Win64 ABI requires argument XMM reg to be copied to the corresponding
         // shadow reg if callee is a varargs function.
         unsigned ShadowReg = 0;
         switch (VA.getLocReg()) {
         case X86::XMM0: ShadowReg = X86::RCX; break;
         case X86::XMM1: ShadowReg = X86::RDX; break;
         case X86::XMM2: ShadowReg = X86::R8; break;
X86TargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
   if (isTailCall) {
     // We used to do:
     //// If this is the first return lowered for this function, add the regs
     //// to the liveout set for the function.
     // This isn't right, although it's probably harmless on x86; liveouts
     // should be computed from returns not tail calls.  Consider a void
     // function making a tail call to a function returning int.
     MF.getFrameInfo().setHasTailCall();
-    return DAG.getNode(X86ISD::TC_RETURN, dl, NodeTys, Ops);
+    SDValue Ret = DAG.getNode(X86ISD::TC_RETURN, dl, NodeTys, Ops);
+    DAG.addCallSiteInfo(Ret.getNode(), std::move(CSInfo));
+    return Ret;
   }
 
   if (HasNoCfCheck && IsCFProtectionSupported) {
     Chain = DAG.getNode(X86ISD::NT_CALL, dl, NodeTys, Ops);
   } else {
     Chain = DAG.getNode(X86ISD::CALL, dl, NodeTys, Ops);
   }
   InFlag = Chain.getValue(1);
+  DAG.addCallSiteInfo(Chain.getNode(), std::move(CSInfo));
 
   // Create the CALLSEQ_END node.
   unsigned NumBytesForCalleeToPop;
   if (X86::isCalleePop(CallConv, Is64Bit, isVarArg,
                        DAG.getTarget().Options.GuaranteedTailCallOpt))
     NumBytesForCalleeToPop = NumBytes;    // Callee pops everything
   else if (!Is64Bit && !canGuaranteeTCO(CallConv) &&
            !Subtarget.getTargetTriple().isOSMSVCRT() &&

test/CodeGen/X86/call-site-info-output.ll

+; Test call site info MIR printer and parser.Parser assertions and machine
+; verifier will check the rest;
+; RUN: llc -debug-entry-values %s -stop-before=expand-isel-pseudos -o %t.mir
+; RUN: cat %t.mir | FileCheck %s
+; CHECK: name: fn2
+; CHECK: callSites:
+; There is no need to verify call instruction location since it will be
+; checked by the MIR parser in the next RUN.
+; CHECK-NEXT: bb: {{.*}}, offset: {{.*}}, fwdArgRegs:
+; CHECK-NEXT:   arg: 0, reg: '$edi'
+; CHECK-NEXT:   arg: 1, reg: '$esi'
+; CHECK-NEXT:   arg: 2, reg: '$edx'
+; RUN: llc -debug-entry-values %t.mir -start-before=expand-isel-pseudos -o -
+
+; ModuleID = 'test/CodeGen/X86/call-site-info-output.c'
+source_filename = "test/CodeGen/X86/call-site-info-output.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: noinline nounwind uwtable
+define dso_local i64 @fn2(i32 %a, i32 %b, i32 %c) local_unnamed_addr #0 {
+entry:
+  %call = tail call i32 (i32, i32, i32, ...) bitcast (i32 (...)* @fn1 to i32 (i32, i32, i32, ...)*)(i32 -50, i32 50, i32 -7) #2
+  %add = mul i32 %a, 3
+  %sub = sub i32 %add, %b
+  %add2 = add i32 %sub, %c
+  %conv4 = sext i32 %add2 to i64
+  ret i64 %conv4
+}
+
+declare dso_local i32 @fn1(...) local_unnamed_addr #1
+
+!llvm.module.flags = !{!0}
+!llvm.ident = !{!1}
+
+!0 = !{i32 1, !"wchar_size", i32 4}
+!1 = !{!"clang version 9.0.0"}