Page MenuHomePhabricator

[Examples] Add IRTransformations directory to examples.
ClosedPublic

Authored by fhahn on Oct 24 2019, 4:43 PM.

Details

Summary

This patch adds a new IRTransformations directory to llvm/examples/. This is
intended to serve as a new home for example transformations/analysis
code used by various tutorials.

If LLVM_BUILD_EXAMPLES is enabled, the ExamplesIRTransforms library is
linked into the opt binary and the example passes become available.

To start off with, it contains the CFG simplifications used in the IR
part of the 'Getting Started With LLVM: Basics' tutorial at the US LLVM
Developers Meeting 2019.

Diff Detail

Event Timeline

fhahn created this revision.Oct 24 2019, 4:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 24 2019, 4:43 PM
fhahn updated this revision to Diff 226359.Oct 24 2019, 4:48 PM

Move struct for new PM pass to .cpp file

andwar added a subscriber: andwar.Oct 24 2019, 8:07 PM
paquette added inline comments.Oct 25 2019, 11:27 AM
llvm/examples/IRTransforms/TutorialSimplifyCFG.cpp
375 ↗(On Diff #226359)

Is there any reason that we don't check the version like in the legacy runOnFunction?

Meinersbur added inline comments.
llvm/tools/opt/opt.cpp
564–566

I'd prefer if the production code stays distinct from examples. In this case D61446 would allow a solution.

I think that having a reference to these examples somewhere in the official docs would be very helpful. Maybe somewhere here: http://llvm.org/docs/WritingAnLLVMPass.html?

[nit] I'd keep the names of files and passes shorter and remove 'Tutorial' from names. IMHO it doesn't really add much in terms of self-documentation, yet makes everything a bit harder to read.

llvm/examples/IRTransforms/TutorialSimplifyCFG.cpp
17 ↗(On Diff #226359)

[nit] Unnecessary empty line.

54 ↗(On Diff #226359)

Commented out code is most likely to become stale - I'd remove it.

58 ↗(On Diff #226359)

[nit] 'a undef' --> 'an undef'

95 ↗(On Diff #226359)

Same as above.

140 ↗(On Diff #226359)

'second first' --> 'first'

176 ↗(On Diff #226359)

'second third' -> 'third'?

177 ↗(On Diff #226359)

[nit] 'use' -> 'uses'

llvm/examples/IRTransforms/TutorialSimplifyCFG.h
17 ↗(On Diff #226359)

[nit] Did you mean initializeTutorialSimplifyCFGLegacyPMPass instead?

fhahn updated this revision to Diff 226887.Oct 29 2019, 6:24 AM
fhahn marked 9 inline comments as done.

Address review comments: removed tutorial from file names, fixed comments, match tutorial version in the NewPM as well. Also added a few TODOs and ideas for future improvements.

fhahn marked an inline comment as done.Oct 29 2019, 6:26 AM
fhahn added inline comments.
llvm/examples/IRTransforms/TutorialSimplifyCFG.cpp
54 ↗(On Diff #226359)

Yes, ideally it should be also uncommented, but I am not entirely sure yet how to best integrate such small variations. I'd like to leave it in, with a todo for now.

375 ↗(On Diff #226359)

Nope, I just missed it! Updated. Although the pass still needs to be hooked up properly so the new PM actually knows about it. I've added a TODO

llvm/examples/IRTransforms/TutorialSimplifyCFG.h
17 ↗(On Diff #226359)

Nope, the INITIALIZE_PASS macros will add a Pass at the end of the pass class name.

llvm/tools/opt/opt.cpp
564–566

I think we should move to the mechanism, once it lands. In the meantime, I think having that in opt should be low risk, as opt is mostly used for testing anyways and the example passes are completely separated from the transforms library.

fhahn added a comment.Oct 29 2019, 6:29 AM

I think that having a reference to these examples somewhere in the official docs would be very helpful. Maybe somewhere here: http://llvm.org/docs/WritingAnLLVMPass.html?

Agreed, we should integrate the new examples in the official docs. WritingAnLLVMPass seems like a good candidate, but currently it seems a bit crowded already and I'd like to think about how to best integrate it a bit more carefully.

[nit] I'd keep the names of files and passes shorter and remove 'Tutorial' from names. IMHO it doesn't really add much in terms of self-documentation, yet makes everything a bit harder to read.

Yep, it should already be obvious from the path and the tutorial part seems redundant. But I have no strong feelings either way.

Includes are missing in SimplifyCFG.h:

cmake -G Ninja -DLLVM_TARGETS_TO_BUILD=X86 -DCMAKE_BUILD_TYPE=Release   -DLLVM_USE_NEWPM=On -DLLVM_BUILD_EXAMPLES=On ../../llvm/
(...)
ninja opt
(...)
/work/llvm-project/llvm/examples/IRTransforms/SimplifyCFG.h:15:1: error: unknown type name 'FunctionPass'
FunctionPass *createSimplifyCFGPass();
^
/work/llvm-project/llvm/examples/IRTransforms/SimplifyCFG.h:17:40: error: unknown type name 'PassRegistry'
void initializeSimplifyCFGLegacyPMPass(PassRegistry &);
                                       ^
llvm/test/Examples/IRTransforms/SimplifyCFG/tut-simplify-cfg-blockaddress.ll
17

[nit] For this example the names of BBs are rather irrelevant, but if.then (or if.else) could suggest otherwise. Maybe worth renaming? Just a thought.

fhahn updated this revision to Diff 227592.Nov 2 2019, 2:38 PM
fhahn marked an inline comment as done.

Fix build error, changed if.then;if.else BB names to bb1 and bb2.

This is great stuff, thank you for doing this!

  1. Why not test with all versions (v1, v2 and v3) in all tests? Does the output change?
  2. AFAIK there are no official guidelines on this (and both approaches are used in practice), but I'd rename SimplifyCFGLegacyPass to SimplifyCFGLegacy (fewer characters, meaning still clear). But please use the one you prefer the most.
  3. I'd prefer to see this fully working with the new PM or no new PM support at all. Otherwise it can be confusing.
llvm/examples/IRTransforms/SimplifyCFG.cpp
11
  1. Could you add a quick summary of your passes (and the variants)? Also, since this a tutorial - maybe also worth mentioning the tests? They are an integral part of this, but this could be non-obvious for beginners.
  2. "implements" twice
  3. Definitely worth adding a link to the video (once it's uploaded)
16

Why not do it now? AFAIK it's been agreed that the new PM is to become the default.

If you prefer to leave it for later then I wouldn't be adding the new PM implementation at all. It's currently not used and not tested.

124

[nit] This is (together with v2 and v3) your least commented function.

373–375

AFAIK, this is neither used nor needed.

llvm/test/Examples/IRTransforms/SimplifyCFG/tut-simplify-cfg2-dead-block-order.ll
45

The following line suggests that BB_2 is always bb.2. You don't need a regex here.

92

[nit] extra line

llvm/test/Examples/IRTransforms/SimplifyCFG/tut-simplify-cfg3-phis.ll
24

The following line suggests that BB_3 is always bb.3.

llvm/test/Examples/IRTransforms/SimplifyCFG/tut-simplify-cfg4-multiple-duplicate-cfg-updates.ll
5

Won't the default version (i.e. v1) invalidate the DominatorTree? (i.e. it won't use DomTreeUpdater).

llvm/test/Examples/IRTransforms/SimplifyCFG/tut-simplify-cfg6-dead-self-loop.ll
19

DELETEME

fhahn updated this revision to Diff 228039.Nov 6 2019, 4:13 AM
fhahn marked 9 inline comments as done.

Thanks. Updated to address the comments.

This is great stuff, thank you for doing this!

  1. Why not test with all versions (v1, v2 and v3) in all tests? Does the output change?

Nope, I just did not update all tests. it's done now :)

  1. AFAIK there are no official guidelines on this (and both approaches are used in practice), but I'd rename SimplifyCFGLegacyPass to SimplifyCFGLegacy (fewer characters, meaning still clear). But please use the one you prefer the most.

I'd prefer to keep it as is for now.

  1. I'd prefer to see this fully working with the new PM or no new PM support at all. Otherwise it can be confusing.

Agreed, I've removed the partial new PM stuff for now.

llvm/examples/IRTransforms/SimplifyCFG.cpp
16

I'd prefer to get a first version in sooner rather than later and do the new PM support as follow-up. I've removed the NewPM stuff in the meantime.

373–375

That's a leftover from living in lib/Transforms. I've had it in the passmanagerbuilder as well.

llvm/test/Examples/IRTransforms/SimplifyCFG/tut-simplify-cfg2-dead-block-order.ll
45

That's what the autogeneration script generates. I agree it's not ideal and we should fix it there.

llvm/test/Examples/IRTransforms/SimplifyCFG/tut-simplify-cfg3-phis.ll
24

Same as earlier, this should be fixed in the generation script.

llvm/test/Examples/IRTransforms/SimplifyCFG/tut-simplify-cfg4-multiple-duplicate-cfg-updates.ll
5

Yes, this is just relevant for the DomTreeUpdater version. It's tested now.

andwar added a comment.EditedNov 7 2019, 1:43 AM

LGTM

Thanks for addressing my comments!

paquette accepted this revision.Nov 11 2019, 2:56 PM

LGTM aside from typos

llvm/examples/IRTransforms/SimplifyCFG.cpp
76

s/remote/remove/

117

s/remote/remove/

157

typo: predecessors

193

typo on predecessors here too

This revision is now accepted and ready to land.Nov 11 2019, 2:56 PM
fhahn marked 4 inline comments as done.Nov 12 2019, 6:14 AM

Thanks for the reviews everyone :)

This revision was automatically updated to reflect the committed changes.
svenvh added a subscriber: svenvh.Nov 19 2019, 2:30 AM
svenvh added inline comments.
llvm/tools/opt/opt.cpp
564–566

This change seems to be causing a problem with the nightly packages from apt.llvm.org. Installing the packages llvm-10-tools llvm-10-dev clang-format-10 clang-tidy-10 libclang-10-dev lldb-10 without installing the examples makes the exported cmake files unusable:

CMake Error at /usr/lib/llvm-10/lib/cmake/llvm/LLVMExports.cmake:1350 (message):
  The imported target "ExampleIRTransforms" references the file

     "/usr/lib/llvm-10/lib/libExampleIRTransforms.a"

  but this file does not exist.  Possible reasons include:

  * The file was deleted, renamed, or moved to another location.

  * An install or uninstall procedure did not complete successfully.

  * The installation package was faulty and contained

     "/usr/lib/llvm-10/lib/cmake/llvm/LLVMExports.cmake"

  but not all the files it references.

Call Stack (most recent call first):
  /usr/lib/llvm-10/cmake/LLVMConfig.cmake:256 (include)
  CMakeLists.txt:29 (find_package)
fhahn marked an inline comment as done.Nov 19 2019, 3:19 AM
fhahn added inline comments.
llvm/tools/opt/opt.cpp
564–566

Thanks, I'll take a look!

svenvh added inline comments.Nov 19 2019, 3:27 AM
llvm/tools/opt/opt.cpp
564–566

After taking a closer look, installing the llvm-10-examples package actually does not solve the problem; cmake still fails with the same error.

fhahn marked an inline comment as done.Nov 22 2019, 3:34 AM
fhahn added inline comments.
llvm/tools/opt/opt.cpp
564–566

I think it should be fixed by D70590, which excludes libExampleIRTransforms.a from the ALL target.

fhahn marked an inline comment as done.Jan 4 2020, 8:16 AM
fhahn added inline comments.
llvm/tools/opt/opt.cpp
564–566

@svenvh I've recommitted the change with a fix that should address the issue. Please let me know if the problem you are seeing re-surfaces.

svenvh added inline comments.Jan 6 2020, 1:42 AM
llvm/tools/opt/opt.cpp
564–566

Thanks @fhahn! It seems fine now, though a similar problem (unrelated to your commit) appeared on the "Bye" example.