Page MenuHomePhabricator

[NFC] Add explicit instantiation to releaseNode
ClosedPublic

Authored by qiucf on Jan 2 2020, 12:05 AM.

Details

Summary

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

Diff Detail

Event Timeline

qiucf created this revision.Jan 2 2020, 12:05 AM

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

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 added a subscriber: lkail.EditedJan 2 2020, 12:36 AM

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 added a comment.Jan 2 2020, 1:17 AM

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 updated this revision to Diff 235834.Jan 2 2020, 1:19 AM
qiucf retitled this revision from [NFC] Put implementation of releaseNode into header file to [NFC] Add explicit instantiation to releaseNode.

Use explicit template instantiation approach.

qiucf marked 2 inline comments as done.Jan 2 2020, 1:20 AM
qiucf added inline comments.
llvm/lib/CodeGen/MachineScheduler.cpp
2097

This helps in both Clang and GCC. Thanks.

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 added a comment.Jan 2 2020, 2:00 AM

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 added inline comments.Jan 2 2020, 2:09 AM
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

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 added inline comments.Jan 2 2020, 3:08 AM
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.

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 added a comment.Jan 2 2020, 3:56 AM

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 accepted this revision.Jan 2 2020, 3:56 AM

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!

This revision is now accepted and ready to land.Jan 2 2020, 3:56 AM
This revision was automatically updated to reflect the committed changes.
qiucf marked an inline comment as done.