Page MenuHomePhabricator

jamieschmeiser (Jamie Schmeiser)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 14 2020, 11:14 AM (5 w, 5 d)

Recent Activity

Tue, Sep 22

jamieschmeiser updated the diff for D87202: Add a new hidden option -dot-cfg-changes which creates a website of DOT files showing colourized changes as the IR is changed by passes in the new pass manager pipeline..

Respond to review comments: use regex, format HTML output to make it easier
to read, other changes.

Tue, Sep 22, 4:03 PM · Restricted Project
jamieschmeiser added a comment to D86360: Add new hidden option -print-changed which only reports changes to IR.

Fair enough, I'll come up with a regex that works for all three. I suspect that this will need to change in response to https://reviews.llvm.org/D87216

Tue, Sep 22, 12:45 PM · Restricted Project

Mon, Sep 21

jamieschmeiser updated the diff for D87000: Add a new hidden option -print-changes that reports in a patch-like notation when a pass actually changes the IR..

Update this PR to match changes made to https://reviews.llvm.org/D86360,
most notably replace the lambdas with virtual functions.
Adapt to other changes based on reviews of -print-changed.

Mon, Sep 21, 5:05 PM · Restricted Project
jamieschmeiser added a comment to D86360: Add new hidden option -print-changed which only reports changes to IR.

@MaskRay, https://reviews.llvm.org/D87000 moves a bunch of the code into a separate file, rather than deleting it. -print-changed, by itself, doesn't add enough to justify it's own file but once you add in -print-changes and -dot-cfg-changes, it grows to a significant amount of code, so I moved it to a separate file. I had already marked the other PRs as children in a chain, which can be seen in the stack tab of the revision contents. Is there another way that I should indicate the relationship between the PRs? Thanks for your comments about these 3 PRs. I have found these ways of looking at the changes very useful, particularly -dot-cfg-changes. (Shameless promotion alert:) I will be presenting these three ways of looking at the IR in my tutorial at the upcoming virtual developers meeting, including a demo of -dot-cfg-changes.

Mon, Sep 21, 5:57 AM · Restricted Project

Fri, Sep 18

jamieschmeiser updated the diff for D86360: Add new hidden option -print-changed which only reports changes to IR.

Respond to review comments: change comments, few code revisions, notably with
std::string. Tighten up test cases with more -NEXT checks of the IR.

Fri, Sep 18, 9:55 AM · Restricted Project
jamieschmeiser added a comment to D86360: Add new hidden option -print-changed which only reports changes to IR.

@ychen, Why are the regexs not sufficient for testing? I would rather not "check the common sub-string of GCC's and MSVC's output" because it has already been sufficiently demonstrated that this is fragile in the test. Referring to the code creating one of the banners:
SmallString<20> Banner = formatv("* IR Dump After {0} *", PassID);
We see that the test checks everything except the PassID (the name is added after) and the rest of the banner is distinct in all cases. I have changed the regexs to {{.+}} which will ensure that there is actually something there but checking anything else will make the test susceptible to failing based on the compiler. The important part of the test is captured in that the banner shows that the dump is either filtered, ignored, output, etc and that the portion of IR is correct. This is the intent of the tests, rather than testing how different compilers construct a PassID.
@MaskRay From earlier comments:
"The reason for the round-trip structure is that there are more instantiations of these templates that report changes in different forms still to come. I will be subsequently posting reviews for at least 2 different change reporters based on these base classes. By defining these templates in this fashion, the functionality of comparing different IR representations and determining when changes have occurred and should be filtered, reported or ignored is all in common in the base classes. The derived classes will be called when interesting events happen and only need to handle presenting the events without having to determine when the events occur."
There are 2 other derived change reporters currently up for review based on these base classes, although the PRs need updating based on changes made in this PR. This makes at least 3 derived classes. See https://reviews.llvm.org/D87000 and https://reviews.llvm.org/D87202

Fri, Sep 18, 9:53 AM · Restricted Project

Thu, Sep 17

jamieschmeiser updated the diff for D86657: Add new hidden option -print-crash-IR that prints out IR that caused opt pipeline to crash.

Respond to review comments: replace unneeded include with standard include, move code in file.
React to review comments on other PRs: change test prefixes to something more useful.
React to test failures with other PRs: change tests to have regex rather than having PassID in test
Add another test to test interaction with -print-module-scope

Thu, Sep 17, 3:05 PM · Restricted Project
jamieschmeiser updated the diff for D86360: Add new hidden option -print-changed which only reports changes to IR.

Respond to build/test problems: Remove extra ;, remove call to base dtor,
change tests to have regex for PassID and ModuleID

Thu, Sep 17, 10:26 AM · Restricted Project
jamieschmeiser reopened D86360: Add new hidden option -print-changed which only reports changes to IR.

It appears that the failing tests are due to differences in the PassID that is passed in the PassInstrumentationCallbacks, which is presumably build compiler/platform dependent. I know that the name sent is language dependent because it is the mangled name for C++ but all of these tests are based on input that was originally C so that should be fine. I will alter the tests to have a regex accepting anything for the PassID and just verify the constructed part of the banner and the name, which should make these tests robust and still test the functionality.

Thu, Sep 17, 8:50 AM · Restricted Project

Wed, Sep 16

jamieschmeiser added a comment to D86360: Add new hidden option -print-changed which only reports changes to IR.

@hliao Thanks!

Wed, Sep 16, 1:52 PM · Restricted Project
jamieschmeiser added a comment to D86360: Add new hidden option -print-changed which only reports changes to IR.

@mkitzan, Thanks for fixing the build by removing the extra semicolon. Also, removing the call to the base dtor is fine.
commit 6859d95ea2d0f3fe0de2923a3f642170e66a1a14
Author: Michael Liao <michael.hliao@gmail.com>
Date: Wed Sep 16 14:43:08 2020 -0400

Wed, Sep 16, 1:28 PM · Restricted Project
jamieschmeiser updated the diff for D86360: Add new hidden option -print-changed which only reports changes to IR.

[NFC] Respond to reviewers comments: Update comments.

Wed, Sep 16, 7:36 AM · Restricted Project

Mon, Sep 14

jamieschmeiser updated the diff for D86360: Add new hidden option -print-changed which only reports changes to IR.

Respond to review comments: change lambdas to virtual function calls and
place code directly in functions.

Mon, Sep 14, 12:26 PM · Restricted Project
jamieschmeiser updated the diff for D86360: Add new hidden option -print-changed which only reports changes to IR.

Respond to review comments: Set ShouldPreserveUseListOrder to true
when generating the output for a module.

Mon, Sep 14, 10:04 AM · Restricted Project
jamieschmeiser added inline comments to D86360: Add new hidden option -print-changed which only reports changes to IR.
Mon, Sep 14, 7:07 AM · Restricted Project

Thu, Sep 10

jamieschmeiser updated the diff for D87202: Add a new hidden option -dot-cfg-changes which creates a website of DOT files showing colourized changes as the IR is changed by passes in the new pass manager pipeline..

Change name to dot-cfg-changes as it is more similar to print-changes than to print-changed

Thu, Sep 10, 12:04 PM · Restricted Project
jamieschmeiser updated the diff for D87202: Add a new hidden option -dot-cfg-changes which creates a website of DOT files showing colourized changes as the IR is changed by passes in the new pass manager pipeline..

Change option name to dot-cfg-changed to align with existing dot-cfg pass.

Thu, Sep 10, 6:50 AM · Restricted Project

Tue, Sep 8

jamieschmeiser updated the diff for D87202: Add a new hidden option -dot-cfg-changes which creates a website of DOT files showing colourized changes as the IR is changed by passes in the new pass manager pipeline..

Changed code to only open output stream when option is specified.

Tue, Sep 8, 7:15 AM · Restricted Project

Sat, Sep 5

jamieschmeiser added a comment to D86362: Allow graph writer to render DOT nodes using HTML..

@madhur13490 I have added a PR https://reviews.llvm.org/D87202 that the contains the code that uses the changes in this PR (and marked this as a parent of it).

Sat, Sep 5, 10:04 PM · Restricted Project
jamieschmeiser requested review of D87202: Add a new hidden option -dot-cfg-changes which creates a website of DOT files showing colourized changes as the IR is changed by passes in the new pass manager pipeline..
Sat, Sep 5, 9:59 PM · Restricted Project

Fri, Sep 4

jamieschmeiser updated the diff for D87000: Add a new hidden option -print-changes that reports in a patch-like notation when a pass actually changes the IR..

Move enum out of template, make space between end template delimiters
optional in new test.

Fri, Sep 4, 3:11 PM · Restricted Project
jamieschmeiser updated the diff for D87000: Add a new hidden option -print-changes that reports in a patch-like notation when a pass actually changes the IR..

This is an update based on changes that have been made to the code in the review
for https://reviews.llvm.org/D86360. I have moved much of the code and templates into
a new header and body files as it is becoming more complex. I have also changed
some of the class names from being "change printer" based to "change reporter"
based because it doesn't imply how the changes are shown (ie, the changes do not
necessarily need to be printed).

Fri, Sep 4, 1:42 PM · Restricted Project
jamieschmeiser updated the diff for D86657: Add new hidden option -print-crash-IR that prints out IR that caused opt pipeline to crash.

Respond to review comments: Change global vector to pointer to DenseSet,
change objects from unnamed namespace to private statics.

Fri, Sep 4, 7:28 AM · Restricted Project

Thu, Sep 3

jamieschmeiser updated the diff for D86360: Add new hidden option -print-changed which only reports changes to IR.

Respond to review comments: Change template parameter name, use
llvm::isSpecialPass, make space between template end delimiters optional
and use more meaningful check prefixes in tests.

Thu, Sep 3, 1:33 PM · Restricted Project
jamieschmeiser reopened D86360: Add new hidden option -print-changed which only reports changes to IR.
Thu, Sep 3, 1:31 PM · Restricted Project
jamieschmeiser added inline comments to D86360: Add new hidden option -print-changed which only reports changes to IR.
Thu, Sep 3, 10:59 AM · Restricted Project
jamieschmeiser added inline comments to D86360: Add new hidden option -print-changed which only reports changes to IR.
Thu, Sep 3, 10:44 AM · Restricted Project
jamieschmeiser updated the diff for D86360: Add new hidden option -print-changed which only reports changes to IR.

Merge of last 2 changesets for committing.

Thu, Sep 3, 8:23 AM · Restricted Project
jamieschmeiser added a comment to D86360: Add new hidden option -print-changed which only reports changes to IR.

Thanks for doing the reviews.

Thu, Sep 3, 7:18 AM · Restricted Project
jamieschmeiser updated the diff for D86360: Add new hidden option -print-changed which only reports changes to IR.

Respond to review comments: fix comment and remove unnecessary test
from isInterestingFunction. Also, mark some functions inline.

Thu, Sep 3, 7:11 AM · Restricted Project

Wed, Sep 2

jamieschmeiser updated the diff for D87000: Add a new hidden option -print-changes that reports in a patch-like notation when a pass actually changes the IR..

Rebase to pick up review changes for https://reviews.llvm.org/D86360 and
alter this code to match those changes (reordered parameters, added const
to parameters)

Wed, Sep 2, 11:01 AM · Restricted Project
jamieschmeiser updated the diff for D86360: Add new hidden option -print-changed which only reports changes to IR.

[NFC] Respond to review comments: change parameter order, change variable
names, add const to parameters

Wed, Sep 2, 10:55 AM · Restricted Project

Tue, Sep 1

jamieschmeiser added a comment to D87000: Add a new hidden option -print-changes that reports in a patch-like notation when a pass actually changes the IR..

Note that this code makes use of the linux diff function to determine what has changed in the IR. This will likely require altering for other platforms, which is a future enhancement. The testing may require attention before this PR can land and any suggestions on how to handle this would be appreciated.

Tue, Sep 1, 8:47 PM · Restricted Project
jamieschmeiser requested review of D87000: Add a new hidden option -print-changes that reports in a patch-like notation when a pass actually changes the IR..
Tue, Sep 1, 8:39 PM · Restricted Project
jamieschmeiser added a comment to D86362: Allow graph writer to render DOT nodes using HTML..

I think you have to be logged in to the llvm conference website to follow the link. The abstract is available on the conference website.

Tue, Sep 1, 11:41 AM · Restricted Project
jamieschmeiser updated the diff for D86360: Add new hidden option -print-changed which only reports changes to IR.

Respond to review comments: remove dummy passes created in last
changeset and change tests to use no-op-function and instsimplify.

Tue, Sep 1, 8:01 AM · Restricted Project
jamieschmeiser updated the diff for D86657: Add new hidden option -print-crash-IR that prints out IR that caused opt pipeline to crash.

Respond to review comments: use mutex to protect building of vector of crash printers.
Create a global vector of crash printers that the signal handler uses to report the IRs in the event of a crash.
Adding a crash printer and removing it from the vector is protected using mutex locking.

Tue, Sep 1, 6:37 AM · Restricted Project

Fri, Aug 28

jamieschmeiser updated the diff for D86360: Add new hidden option -print-changed which only reports changes to IR.

Create 2 dummy passes, one that changes the IR by adding a dummy attribute
to a function, the other which does not change the IR. Neither are
added to any pipeline. Change the -print-changed unit test to use these
2 passes to make the unit test robust.

Fri, Aug 28, 2:36 PM · Restricted Project
jamieschmeiser updated the diff for D86657: Add new hidden option -print-crash-IR that prints out IR that caused opt pipeline to crash.

Address review comments.

Fri, Aug 28, 11:57 AM · Restricted Project
jamieschmeiser added a comment to D86362: Allow graph writer to render DOT nodes using HTML..

Ping

Fri, Aug 28, 11:04 AM · Restricted Project

Thu, Aug 27

jamieschmeiser added inline comments to D86657: Add new hidden option -print-crash-IR that prints out IR that caused opt pipeline to crash.
Thu, Aug 27, 2:07 PM · Restricted Project
jamieschmeiser added a comment to D86657: Add new hidden option -print-crash-IR that prints out IR that caused opt pipeline to crash.

For a test, I could create a pass that calls __builtin_trap that is only added to the opt pipeline by a really hidden option. The danger is that if someone uses that option, the compiler would crash, which seems dangerous just for testing. Is that desirable?

Thu, Aug 27, 2:04 PM · Restricted Project

Wed, Aug 26

jamieschmeiser updated the diff for D86657: Add new hidden option -print-crash-IR that prints out IR that caused opt pipeline to crash.

[NFC] Reorder includes and change case of variable as suggested by
clang-tidy/clang-format.

Wed, Aug 26, 7:58 PM · Restricted Project
jamieschmeiser added a comment to D86657: Add new hidden option -print-crash-IR that prints out IR that caused opt pipeline to crash.

--print-before-all will print the IR before every pass which can result in the IR being printed literally thousands of times while this option prints it only once. The IR is saved in a string which is cleared each time and re-used so the memory overhead is not great; also this overhead is only hit when the option is requested. It does slow the compilation when used but not as much as --print-before-all and the user does not have as difficult a task in extracting the IR from the output. If the option is false (the default), the overhead is only the checking of the option. Basically, --print-before-all is cumbersome for this purpose while this option is convenient. Also, one could reasonably request a user to retrieve the IR (if they are willing) with this option when reporting a crash while requesting a user to use --print-before-all would not be reasonable.

Wed, Aug 26, 7:14 PM · Restricted Project
jamieschmeiser added a comment to D86657: Add new hidden option -print-crash-IR that prints out IR that caused opt pipeline to crash.

This option will be introduced (along with others) in my tutorial at the LLVM developer's conference in October.

Wed, Aug 26, 1:28 PM · Restricted Project
jamieschmeiser requested review of D86657: Add new hidden option -print-crash-IR that prints out IR that caused opt pipeline to crash.
Wed, Aug 26, 1:26 PM · Restricted Project
jamieschmeiser added a comment to D86360: Add new hidden option -print-changed which only reports changes to IR.

See comments regarding the unit tests

Wed, Aug 26, 12:17 PM · Restricted Project

Tue, Aug 25

jamieschmeiser added inline comments to D86360: Add new hidden option -print-changed which only reports changes to IR.
Tue, Aug 25, 3:42 PM · Restricted Project

Aug 24 2020

jamieschmeiser added a comment to D86360: Add new hidden option -print-changed which only reports changes to IR.

Thanks for the review comments. I have tried to address all of your concerns above and in the last set of changes posted.

Aug 24 2020, 1:58 PM · Restricted Project
jamieschmeiser updated the diff for D86360: Add new hidden option -print-changed which only reports changes to IR.

Make IRChangePrinter a member of StandardInstrumentations to control
object lifespan.

Aug 24 2020, 1:29 PM · Restricted Project
jamieschmeiser added a comment to D86360: Add new hidden option -print-changed which only reports changes to IR.

I noticed a problem in the code and posted the fix separately as I did not want to confuse it with the changes based on the review comments.

Aug 24 2020, 11:42 AM · Restricted Project
jamieschmeiser updated the diff for D86360: Add new hidden option -print-changed which only reports changes to IR.

Always stack an entry on the stack before each pass in change reporters.

Aug 24 2020, 11:40 AM · Restricted Project

Aug 22 2020

jamieschmeiser updated the diff for D86360: Add new hidden option -print-changed which only reports changes to IR.

Change function names based on suggestions from clang-tidy

Aug 22 2020, 3:38 PM · Restricted Project
jamieschmeiser updated the diff for D86362: Allow graph writer to render DOT nodes using HTML..

Incorporate formatting changes suggested by clang-format/clang-tidy.

Aug 22 2020, 6:04 AM · Restricted Project

Aug 21 2020

jamieschmeiser requested review of D86362: Allow graph writer to render DOT nodes using HTML..
Aug 21 2020, 12:32 PM · Restricted Project
jamieschmeiser requested review of D86360: Add new hidden option -print-changed which only reports changes to IR.
Aug 21 2020, 11:10 AM · Restricted Project

Aug 14 2020

jamieschmeiser requested review of D85999: [NFC] Add raw_ostream parameter to printIR routines.
Aug 14 2020, 2:43 PM · Restricted Project