Page MenuHomePhabricator

[CodeGen] Add extension points for TargetPassConfig::addMachinePasses
Needs ReviewPublic

Authored by drti on Mar 13 2021, 3:25 PM.

Details

Reviewers
ychen
arsenm
Summary

Allow out-of-tree code to customize a target using a
mechanism similar to addGlobalExtension in PassManagerBuilder but for
machine passes.

Diff Detail

Event Timeline

drti created this revision.Mar 13 2021, 3:25 PM
drti requested review of this revision.Mar 13 2021, 3:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 13 2021, 3:25 PM
drti added a comment.Mar 13 2021, 3:29 PM

Hi this is my first submission to the LLVM project. I selected reviewers by looking at the recent commit history of TargetPassConfig.h so I hope I haven't gone too far wrong with that. Evan Cheng is listed in the CODE_OWNERS.txt for "parts of code generator not covered by someone else" but I couldn't find his user name on Phabricator (perhaps he doesn't have one).

ychen added a comment.Mar 15 2021, 4:00 PM

Could you add an example like llvm/examples/Bye and a test using llc -load?

llvm/include/llvm/CodeGen/TargetPassConfig.h
370

Why is Target needed? A plugin could query TargetPassConfig about which target it wants to register itself.

llvm/unittests/CodeGen/TargetExtensionTest.cpp
54

I think testing the default target should be enough. Why we test all targets here?

drti added a comment.Mar 16 2021, 1:19 PM

Thanks very much @ychen for looking at my review, and thanks for pointing me at the examples/Bye/Bye.cpp example code. I didn't know we could do that kind of thing in the build and I'll post an update with a new example later this week.

I see your other two comments as being closely related to each other. If we handle target filtering in the TargetPassConfig implementation then it makes sense for the test to confirm that the filtering works, i.e. an extension registered for one target is not mistakenly invoked for a different one. If we put the responsibility on the client code to filter out unwanted targets then the test wouldn't have to care and any target machine (so long as it can be cast to LLVMTargetMachine, of course) would do.

I can see one benefit to doing the filtering in the implementation rather than the client - in makes it obvious that the filtering is necessary, i.e. the API forces the client code to select a specific target. Otherwise the client code could omit the filtering and then it would work during developer testing say, but crash mysteriously if invoked on an unsuitable target. Maybe that's unlikely, so please let me know if you'd prefer me to simpilfy the implementation by removing the filtering.

ychen added a comment.Mar 17 2021, 9:24 AM

so please let me know if you'd prefer me to simpilfy the implementation by removing the filtering.

Yeah, I still think the filtering should be removed. The plugin should not reply on a Target instance to work and ideally, the plugin should be able to cope with more than one target (most likely a machine-independent pass).

drti added a comment.Mar 25 2021, 2:07 PM

Could you add an example like llvm/examples/Bye and a test using llc -load?

I've made the changes to remove the filtering and simplify the test but I'm having some trouble with the Bye example. When I enable this via cmake ... -DLLVM_BYE_LINK_INTO_TOOLS=ON I get some test failures from llvm-check. Is that expected?

Failed Tests (6):

LLVM :: Other/new-pm-O0-defaults.ll
LLVM :: Other/opt-O0-pipeline.ll
LLVM :: Other/opt-O2-pipeline.ll
LLVM :: Other/opt-O3-pipeline-enable-matrix.ll
LLVM :: Other/opt-O3-pipeline.ll
LLVM :: Other/opt-Os-pipeline.ll

Example error output:

  • TEST 'LLVM :: Other/opt-O3-pipeline-enable-matrix.ll' FAILED ****

Script:

: 'RUN: at line 1'; /home/raoul/build/llvm/head-static/bin/opt -enable-new-pm=0 -O3 -enable-matrix -debug-pass=Structure < /home/raoul/src/llvm/llvm-project/llvm/test/Other/opt-O3-pipeline-enable-matrix.ll -o /dev/null 2>&1 | /home/raoul/build/llvm/head-static/bin/FileCheck /home/raoul/src/llvm/llvm-project/llvm/test/Other/opt-O3-pipeline-enable-matrix.ll

Exit Code: 1

Command Output (stderr):

/home/raoul/src/llvm/llvm-project/llvm/test/Other/opt-O3-pipeline-enable-matrix.ll:216:15: error: CHECK-NEXT: is not on the line after the previous match
; CHECK-NEXT: Canonicalize natural loops

^

<stdin>:212:2: note: 'next' match was here
Canonicalize natural loops
^
<stdin>:208:11: note: previous match ended here
Early CSE

^

<stdin>:209:1: note: non-matching line after previous match is here
Good Bye World Pass
^

Input file: <stdin>
Check file: /home/raoul/src/llvm/llvm-project/llvm/test/Other/opt-O3-pipeline-enable-matrix.ll

-dump-input=help explains the following input dump.

Input was:
<<<<<<

  .
  .
  .
207:  Lower the matrix intrinsics 
208:  Early CSE 
209:  Good Bye World Pass 
210:  Dominator Tree Construction 
211:  Natural Loop Information 
212:  Canonicalize natural loops

next:216 !~~~~~~~~~~~~~~~~~~~~~~~~~ error: match on wrong line

213:  LCSSA Verifier 
214:  Loop-Closed SSA Form Pass 
215:  Basic Alias Analysis (stateless AA impl) 
216:  Function Alias Analysis Results 
217:  Scalar Evolution Analysis 
  .
  .
  .
drti updated this revision to Diff 337882.Apr 15 2021, 1:16 PM

[CodeGen] Simplify unit test for TargetPassConfig::addExtension and
add example plugin module

The example MachineExtension plugin module has a lit test which runs
if examples are enabled.

drti added a comment.Apr 15 2021, 1:20 PM

@ychen I've updated this review with your suggested changes. I've removed the extension filtering in TargetPassConfig, simplified the test and added an example. The example doesn't try to use the new pass manager because the last time I checked it still wasn't useful for machine passes. What do you think?

@drti hi, could you please rebase the patch? The base commit seems not in the git history.

drti updated this revision to Diff 338450.Apr 19 2021, 2:13 AM

[CodeGen] Add extension points for TargetPassConfig::addMachinePasses

Allow out-of-tree code to customize a target using a
mechanism similar to addGlobalExtension in PassManagerBuilder but for
machine passes.

drti added a comment.Apr 19 2021, 2:19 AM

@ychen apologies for that, the base revision should now be efc013ec4d950a68e6f80dd98cda35e1a96a6fc8 from the main repo.

drti updated this revision to Diff 344053.May 10 2021, 7:27 AM
[CodeGen] Add extension points for TargetPassConfig::addMachinePasses

Allow out-of-tree code to customize a target using a
mechanism similar to addGlobalExtension in PassManagerBuilder but for
machine passes.
drti updated this revision to Diff 345072.May 13 2021, 2:04 AM

[CodeGen] Add extension points for TargetPassConfig::addMachinePasses

Allow out-of-tree code to customize a target using a
mechanism similar to addGlobalExtension in PassManagerBuilder but for
machine passes.

drti updated this revision to Diff 345076.May 13 2021, 2:39 AM

[CodeGen] Add extension points for TargetPassConfig::addMachinePasses

Allow out-of-tree code to customize a target using a
mechanism similar to addGlobalExtension in PassManagerBuilder but for
machine passes.

drti marked 2 inline comments as done.May 13 2021, 12:22 PM

@ychen I've rebased and fixed some pre-merge build and formatting issues. Could you please have another look at this review? If it looks OK could I also ask you to commit it for me, please? It's my first submission and I don't have commit rights myself.
Regards,
Raoul

ychen added inline comments.Jun 16 2021, 5:14 PM
llvm/examples/Bye/CMakeLists.txt
10

Why this?

llvm/examples/CMakeLists.txt
11

I think it's better to call this MachineBye to reflect that the usage/implementation are similar between the two. If you think it makes sense, make sure the class name is also renamed accordingly.

llvm/examples/MachineExtension/MachineExtension.cpp
33 ↗(On Diff #345076)

No need for anything concrete there. I think printing some string using errs() should be enough.

62 ↗(On Diff #345076)

Better to use lambda for this one-time registration.

llvm/include/llvm/CodeGen/TargetPassConfig.h
91

How about reducing these extension points to a list that very likely to be useful? Unlike IR pipeline, the order among machine passes is pretty firm, if there are no concrete use cases for an extension point, it may become dead code, or we would need to move/rename it in the future.

509

I'd prefer to call it addMachinePassExtentions.

522

format

533

format

llvm/include/llvm/Target/TargetMachine.h
159

asLLVMTargetMachine is not needed. If you could just static_cast to LLVMTargetMachine directly since TargetMachine object is almost never created.

drti added inline comments.Jun 17 2021, 2:05 PM
llvm/examples/Bye/CMakeLists.txt
10

On my Linux system at least (Fedora 33) the Bye example fails to build when configured with -DBUILD_SHARED_LIBS=true. Strictly speaking this belongs in a separate review but seeing as I'm fixing it for the new example I thought I'd sneak this one in.

The failures is because -zdefs is enabled via CMAKE_SHARED_LINKER_FLAGS in llvm/cmake/modules/HandleLLVMOptions.cmake - this linker flag effectively introduces the same restriction against undefined symbols in shared objects as applies to DLLs on Windows (where the Bye example is disabled)

Example error:
examples/Bye/CMakeFiles/Bye.dir/Bye.cpp.o:Bye.cpp:vtable for (anonymous namespace)::LegacyBye: error: undefined reference to 'llvm::Pass::getPassName() const'

llvm/include/llvm/CodeGen/TargetPassConfig.h
91

I agree that we should limit the number of extension points, but I think we also should try to cater for a range of possible usages. The list at the moment more-or-less mirrors the virtual function extension points for the TargetPassConfig derived classes, which I thought was a fair compromise. There's a tradeoff here of course, but I would tend to err on the side of having more rather than fewer, simply because it's a high barrier for someone working on out-of-tree code to get an extension point added if necessary, compared to an in-tree change that needs, say, a new virtual function extension point.

522

This was auto-formatted this way, either during arcanist processing or to satisfy some lint/format warnings. I'd be hesitant to change this, even though I agree it doesn't look "right".

533

Also auto-formatted this way.

llvm/include/llvm/Target/TargetMachine.h
159

I've seen quite a few places in the code doing the exact unchecked static_cast you suggest, but I hadn't wanted to follow that pattern myself. I didn't think this base class warrants the full LLVM RTTI support, and the virtual function seemed like an easy solution. I had intended to submit another review to introduce it in the existing use-cases that are simple enough (see, e.g. the related TODO comment in llvm/tools/llvm-exegesis/lib/Assembler.cpp and FIXME in llvm/tools/opt/opt.cpp)

drti updated this revision to Diff 354853.Jun 28 2021, 5:24 AM

[CodeGen] Add extension points for TargetPassConfig::addMachinePasses

Allow out-of-tree code to customize a target using a
mechanism similar to addGlobalExtension in PassManagerBuilder but for
machine passes.

drti added inline comments.Jun 28 2021, 5:25 AM
llvm/include/llvm/CodeGen/TargetPassConfig.h
509

Well my reasoning was that addExtension "adds" an extension function and "apply" runs the functions (if any). It may also be that a callback doesn't add any passes for a particular target so strictly speaking all we're doing is invoking the callbacks. What do you think?

drti marked an inline comment as done.Jun 28 2021, 5:26 AM
drti added a comment.Mon, Sep 13, 5:00 AM

@ychen could you please take another look at this review? I'll understand if you don't have time for this - perhaps you could let me know if I should look for another reviewer. It's been quite a while now...