[MS] Pass S_HEAPALLOCSITE metadata through SelectionDAG

[akhuang]

Summary

This emits CodeView debug info for heapallocsite by emitting labels around
the call instructions. The metadata is passed through instruction selection by mapping SDNodes to metadata.
https://reviews.llvm.org/D60800 is a previous change to emit the metadata
when FastISel is used.

Related to bug https://bugs.llvm.org/show_bug.cgi?id=38491

This is a recommit of https://reviews.llvm.org/D61105

[rnk]

We discussed this offline, and after looking at rL364516, it seems like the side table might be a better way to go.

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp

789–790

I don't think we should assume that any metadata node on a call is a heap alloc site.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

3390–3391 ↗

(On Diff #210912)

Given that this is hot code, I think we want to avoid scanning operands if possible. Can we only do this for variadic instructions, like calls?

[akhuang]

The code was adding the metadata to the wrong node in the tail call path; now fixed.

[akhuang]

clang format

[rnk]

llvm/lib/Target/X86/X86ISelLowering.cpp

4061

Code golf suggestion, saves a hash lookup:

if (CLI.CS)
  if (MDNode *HeapAlloc = CLI.CS->getMetatata("heapallocsite"))
    DAG.addHeapAllocSite(Ret.getNode(), HeapAlloc);

4062

I thought testing showed that MSVC didn't emit these records for tail calls. If we simply do nothing here, we'd skip the S_HEAPALLOCSITE record, and things would keep working, right? Any reason not to do that?

4078

Same suggestion

[akhuang]

Remove tail call metadata; update test

[akhuang]

llvm/lib/Target/X86/X86ISelLowering.cpp

4062

Yeah, not sure why I added it back here. I removed it, but some invalid symbol redefinitions are still occurring.

[akhuang]

• Add clear to the SDNode map when Selection DAG is cleared

[rnk]

lgtm, let's try again...

[akhuang]

Committed in https://reviews.llvm.org/rL368230

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp

ProcessSourceNode(SDNode *N, SelectionDAG *DAG, InstrEmitter &Emitter,
   // Even if no instruction was generated, a Value may have become defined via
   // earlier nodes. Try to process them now.
   ProcessSDDbgValues(N, DAG, Emitter, Orders, VRBaseMap, Order);
 }
 
 void ScheduleDAGSDNodes::
 EmitPhysRegCopy(SUnit *SU, DenseMap<SUnit*, unsigned> &VRBaseMap,
                 MachineBasicBlock::iterator InsertPos) {
   for (SUnit::const_pred_iterator I = SU->Preds.begin(), E = SU->Preds.end();
        I != E; ++I) {
     if (I->isCtrl()) continue;  // ignore chain preds
     if (I->getSUnit()->CopyDstRC) {
       // Copy to physical register.
       DenseMap<SUnit*, unsigned>::iterator VRI = VRBaseMap.find(I->getSUnit());
       assert(VRI != VRBaseMap.end() && "Node emitted out of order - late");
       // Find the destination physical register.
       unsigned Reg = 0;
       for (SUnit::const_succ_iterator II = SU->Succs.begin(),
for (SDNode *N = SU->getNode()->getGluedNode(); N; N = N->getGluedNode())
       GluedNodes.push_back(N);
     while (!GluedNodes.empty()) {
       SDNode *N = GluedNodes.back();
       auto NewInsn = EmitNode(N, SU->OrigNode != SU, SU->isCloned, VRBaseMap);
       // Remember the source order of the inserted instruction.
       if (HasDbg)
         ProcessSourceNode(N, DAG, Emitter, VRBaseMap, Orders, Seen, NewInsn);
 
+      // if (NewInsn && NewInsn->isCall())
+        // NewInsn->dump();
+
       if (MDNode *MD = DAG->getHeapAllocSite(N)) {
-        if (NewInsn && NewInsn->isCall())
+        if (NewInsn && NewInsn->isCall()) {
           MF.addCodeViewHeapAllocSite(NewInsn, MD);
         }
+      }
 
       GluedNodes.pop_back();
     }
     auto NewInsn =
         EmitNode(SU->getNode(), SU->OrigNode != SU, SU->isCloned, VRBaseMap);
     // Remember the source order of the inserted instruction.
     if (HasDbg)
       ProcessSourceNode(SU->getNode(), DAG, Emitter, VRBaseMap, Orders, Seen,

llvm/lib/Target/X86/X86ISelLowering.cpp

if (isTailCall) {
     //// 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();
     SDValue Ret = DAG.getNode(X86ISD::TC_RETURN, dl, NodeTys, Ops);
     DAG.addCallSiteInfo(Ret.getNode(), std::move(CSInfo));
-    if (CLI.CS && CLI.CS->getMetadata("heapallocsite")) {
-      DAG.addHeapAllocSite(Chain.getNode(),
+    if (CLI.CS && CLI.CS->getMetadata("heapallocsite"))
+      DAG.addHeapAllocSite(Ret.getNode(),
                            CLI.CS->getMetadata("heapallocsite"));
-    }
 
     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));
 
   // Save heapallocsite metadata.
-  if (CLI.CS && CLI.CS->getMetadata("heapallocsite")) {
+  if (CLI.CS && CLI.CS->getMetadata("heapallocsite"))
     DAG.addHeapAllocSite(Chain.getNode(),
                          CLI.CS->getMetadata("heapallocsite"));
-  }
 
   // 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() &&