[NFC] Add explicit instantiation to releaseNode

[qiucf]

Summary

This patch resolves a build failure about undefined symbols introduced by f9f78cf, when building with older versions of GCC.

[merge_guards_bot]

Unit tests: pass. 61168 tests passed, 0 failed and 728 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[steven.zhang]

If you want to pass it with parameter, why not change the signature of releaseNode directly instead of adding a wrapper. We need to make decision about template arguments vs function parameter :)

[lkail]

Can you add more info about compilation errors and compiler version?

llvm/include/llvm/CodeGen/MachineScheduler.h

762 ↗

(On Diff #235832)

I think adding an Impl method might break origin author's intention. With a template parameter, compiler can remove some dead branches at compile time.

llvm/lib/CodeGen/MachineScheduler.cpp

2097

Can template explicit instantiation help? By adding

template void SchedBoundary::releaseNode<true>(SUnit *SU, unsigned ReadyCycle,                                                                                                                 
                                               unsigned Idx);                                                                                                                                  
template void SchedBoundary::releaseNode<false>(SUnit *SU, unsigned ReadyCycle,                                                                                                                
                                                unsigned Idx);

before this line.

[qiucf]

In D72069#1800876, @lkail wrote:

Can you add more info about compilation errors and compiler version?

Yes. I'm using GCC 8.2.1 20180813 (Advance-Toolchain-at12.0) [revision 263510]. The error information is:

bin/ld: lib/libLLVMPowerPCCodeGen.a(PPCMachineScheduler.cpp.o): in function `llvm::PostGenericScheduler::releaseTopNode(llvm::SUnit*)':
PPCMachineScheduler.cpp:(.text._ZN4llvm20PostGenericScheduler14releaseTopNodeEPNS_5SUnitE[_ZN4llvm20PostGenericScheduler14releaseTopNodeEPNS_5SUnitE]+0x2c): undefined reference to `void llvm::SchedBoundary::releaseNode<false>(llvm::SUnit*, unsigned int, unsigned int)'

bin/ld: lib/libLLVMAMDGPUCodeGen.a(GCNSchedStrategy.cpp.o): in function `llvm::GenericScheduler::releaseBottomNode(llvm::SUnit*)':
GCNSchedStrategy.cpp:(.text._ZN4llvm16GenericScheduler17releaseBottomNodeEPNS_5SUnitE[_ZN4llvm16GenericScheduler17releaseBottomNodeEPNS_5SUnitE]+0x34): undefined reference to `void llvm::SchedBoundary::releaseNode<false>(llvm::SUnit*, unsigned int, unsigned int)'

bin/ld: lib/libLLVMAMDGPUCodeGen.a(GCNSchedStrategy.cpp.o): in function `llvm::GenericScheduler::releaseTopNode(llvm::SUnit*)':
GCNSchedStrategy.cpp:(.text._ZN4llvm16GenericScheduler14releaseTopNodeEPNS_5SUnitE[_ZN4llvm16GenericScheduler14releaseTopNodeEPNS_5SUnitE]+0x34): undefined reference to `void llvm::SchedBoundary::releaseNode<false>(llvm::SUnit*, unsigned int, unsigned int)'

collect2: Error：ld returns 1
ninja: build stopped: subcommand failed.

[qiucf]

Use explicit template instantiation approach.

[qiucf]

llvm/lib/CodeGen/MachineScheduler.cpp

2097

This helps in both Clang and GCC. Thanks.

[merge_guards_bot]

Unit tests: pass. 61168 tests passed, 0 failed and 728 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[fhahn]

In D72069#1800869, @steven.zhang wrote:

If you want to pass it with parameter, why not change the signature of releaseNode directly instead of adding a wrapper. We need to make decision about template arguments vs function parameter :)

+1. I think just passing InPQueue would keep things simpler and it should get inlined anyways ;)

[DoktorC]

llvm/include/llvm/CodeGen/MachineScheduler.h

762 ↗

(On Diff #235832)

You got the point @lkail. For a (verbose) explanation of my intent, refer to my last comment
https://reviews.llvm.org/D65506#1800921

[steven.zhang]

In D72069#1800924, @fhahn wrote:

In D72069#1800869, @steven.zhang wrote:

If you want to pass it with parameter, why not change the signature of releaseNode directly instead of adding a wrapper. We need to make decision about template arguments vs function parameter :)

+1. I think just passing InPQueue would keep things simpler and it should get inlined anyways ;)

+1

[lkail]

llvm/include/llvm/CodeGen/MachineScheduler.h

762 ↗

(On Diff #235832)

But to clarify, I'm not a fan of separating template's def and decl, since it's ABI error-prone. I would let other decide what is an proper implementation here, considering code standard of LLVM doesn't limit scope of using template.

[teemperor]

Note that this doesn't just affect older GCC versions but also everything that runs Apple Clang + LLVM_ENABLE_MODULES (which breaks for example all LLDB builds on Green Dragon and my local builds). So I would prefer if we could land this sooner than later.

[fhahn]

In D72069#1801015, @teemperor wrote:

Note that this doesn't just affect older GCC versions but also everything that runs Apple Clang + LLVM_ENABLE_MODULES (which breaks for example all LLDB builds on Green Dragon and my local builds). So I would prefer if we could land this sooner than later.

@qiucf are you happy to update this to just pass the IsPending as regular argument? Otherwise I'll go ahead and commit this to unblock the bots.

[DoktorC]

As pointed out by @teemperor, it would be better to fix the error as soon as possible.

I see your point @fhahn, @steven.zhang e and @lkail, eventhough I'm not totally convinced
about correct inlining by the compiler (but these are words of a newbie, and, unfortunately, I
do not have the possibilty to personally check if inlining is performed).

Since the build passed, we may let land this patch to fix any trouble caused, and carefully plan
to do a proper follow up with function argument, instead of template argument.

This LGTM! Thank you all!

llvm/lib/CodeGen/MachineScheduler.cpp

if (OtherCritIdx) {
     LLVM_DEBUG(
         dbgs() << "  " << Available.getName() << " + Remain CritRes: "
                << OtherCritCount / SchedModel->getResourceFactor(OtherCritIdx)
                << " " << SchedModel->getResourceName(OtherCritIdx) << "\n");
   }
   return OtherCritCount;
 }
 
+template void SchedBoundary::releaseNode<true>(SUnit *SU, unsigned ReadyCycle,
+                                               unsigned Idx);
+template void SchedBoundary::releaseNode<false>(SUnit *SU, unsigned ReadyCycle,
+                                                unsigned Idx);
+
 template <bool InPQueue>
 void SchedBoundary::releaseNode(SUnit *SU, unsigned ReadyCycle, unsigned Idx) {
   assert(SU->getInstr() && "Scheduled SUnit must have instr");
 
 #ifndef NDEBUG
   // ReadyCycle was been bumped up to the CurrCycle when this node was
   // scheduled, but CurrCycle may have been eagerly advanced immediately after
   // scheduling, so may now be greater than ReadyCycle.
   if (ReadyCycle > CurrCycle)
     MaxObservedStall = std::max(ReadyCycle - CurrCycle, MaxObservedStall);