This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Add extension points for TargetPassConfig::addMachinePasses
AcceptedPublic

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

Details

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
367

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 ↗(On Diff #345076)

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.

505

I'd prefer to call it addMachinePassExtentions.

518

format

529

format

llvm/include/llvm/Target/TargetMachine.h
170

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 Diff #345076)

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.

518

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".

529

Also auto-formatted this way.

llvm/include/llvm/Target/TargetMachine.h
170

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
505

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.Sep 13 2021, 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...

Herald added a project: Restricted Project. · View Herald TranscriptNov 16 2022, 4:42 PM
arsenm requested changes to this revision.Nov 16 2022, 4:42 PM

I'm sure this needs a rebase. Should also be reviewed by out of tree target maintainers

This revision now requires changes to proceed.Nov 16 2022, 4:42 PM
drti planned changes to this revision.Nov 22 2022, 1:27 PM

Hi @arsenm thanks for taking a look at this. I'm in the process of rebasing and re-testing but it will take me a little while to get back up to speed on this. I think it makes sense to put it in the "Plan Changes" stage for now.

Regards,
@drti

drti updated this revision to Diff 478202.Nov 28 2022, 5:36 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 480018.Dec 5 2022, 1:40 AM

[CodeGen] Add extension points for TargetPassConfig::addMachinePasses

Rebase to see if the lljit-with-thinlto-summaries.test has been fixed

drti added a comment.Dec 10 2022, 7:48 AM

@dsanders I think this is in a good state for review again, could you please have a look? There were some outstanding comments from @ychen earlier which you might want to consider (e.g. he thought we needed fewer extension points). Thanks @drti

arsenm added inline comments.Dec 21 2022, 4:21 PM
llvm/include/llvm/Target/TargetMachine.h
440

I don't understand the point of this, there's no cast here

llvm/lib/CodeGen/TargetPassConfig.cpp
730

Is removal ever really needed?

drti added inline comments.Dec 24 2022, 2:39 AM
llvm/include/llvm/Target/TargetMachine.h
440

This overrides the baseclass version of the function which returns nullptr. I think ideally we'd have proper dyn_cast<LLVMTargetMachine> support but the TargetMachine base class isn't set up for this. The virtual function here is just an easy way to get safer downcasting rather than client code using an unchecked static_cast. @ychen was also questioning the utility of this, by the way, suggesting to use the static_cast like some existing code already does. What do you think?

llvm/lib/CodeGen/TargetPassConfig.cpp
730

I guess the only situation where it's useful is if shared objects/DLLs are dynamically unloaded - any registered extension would crash if not removed before unloading its code.

drti marked 2 inline comments as done.Jan 26 2023, 9:06 AM

Hi @arsenm I'm not really sure what the process is here. I'm happy to change the code just wanted some clarification on your opinion - have commented on the issues you raised.

arsenm accepted this revision.Jun 22 2023, 10:34 AM

Seems plausible. Hopefully this won't get too in the way of new PM codegen support

llvm/include/llvm/CodeGen/TargetPassConfig.h
100

Can this be function_ref?

518

Might as well try hitting this with clang-format again

This revision is now accepted and ready to land.Jun 22 2023, 10:34 AM
drti updated this revision to Diff 541002.Jul 17 2023, 6:49 AM

Rebase to latest LLVM, fix formatting issues

drti updated this revision to Diff 542667.Jul 20 2023, 2:09 PM
drti marked an inline comment as done.

Stop test running on Windows using the new UNSUPPORTED syntax

drti added a comment.Jul 23 2023, 12:15 PM

Hi @arsenm I've rebased this so it builds against the current HEAD and addressed a couple of inline comments. I don't have commit rights myself, but if you're happy with the current state could you please merge it for me or let me know what the next step is otherwise?

Regards,
@drti

llvm/include/llvm/CodeGen/TargetPassConfig.h
100

I don't think it can in general because we need to store a list of these and the function_ref docs say "This class does not own the callable, so it is not in general safe to store a function_ref"