Page MenuHomePhabricator

Fix IRPrinting bug
ClosedPublic

Authored by tyb0807 on Jun 28 2018, 7:46 AM.

Details

Summary

Hello all,

Currently, all IRPrinting passes are not considered as analysis, so when -print-after/before-all option enabled, we'll print the IR twice in a row. Note that we do not have this issue with BitcodeWriter pass, since it is already an analysis pass. For example, with:

$ opt -O1 -print-after-all -debug-pass=Arguments -o /dev/null

we got

Pass Arguments:  -targetlibinfo -print-module ... -write-bitcode

whereas with

$ opt -O1 -print-after-all -debug-pass=Arguments -S -o /dev/null

we got

Pass Arguments:  -targetlibinfo -print-module ... -print-module -print-module

This patch will remove the second -print-module pass from the pipeline.

Diff Detail

Repository
rL LLVM

Event Timeline

tyb0807 created this revision.Jun 28 2018, 7:46 AM
tyb0807 edited the summary of this revision. (Show Details)Jun 28 2018, 7:49 AM
tyb0807 edited the summary of this revision. (Show Details)
dexonsmith requested changes to this revision.Jun 28 2018, 9:02 AM

This seems like a reasonable thing to do. Please add tests to check the output of both pass managers.

This revision now requires changes to proceed.Jun 28 2018, 9:02 AM

Thanks for taking a look at this. However, IIRC, the new pass manager does not have print-before/after(-all) mode yet. That is the next thing that I want to do (if you agree, of course)

I'm not sure this technique will work in the new pass manager, since analyses are run on-demand IIRC. I wonder if it would be better to leave these as passes, and implement -print-after-all by inserting -print-module/etc. passes into the pipeline as the pipeline is being constructed. You could avoid adding extra -print-module statements in cases where that pass already exists.

In any case you'll need a test for what you have changed.

tyb0807 updated this revision to Diff 153494.Jun 29 2018, 7:36 AM

No don't get me wrong, this patch is supposed to fix the issue in the LegacyPassManager, and I do not intend to change anything about the new pass manager in this patch.

As to implementing print-after-all in the new pass manager, I do not know what should be the best way to do yet... Your idea seems to be a good solution.

Anyway, tests added for this patch.

No don't get me wrong, this patch is supposed to fix the issue in the LegacyPassManager, and I do not intend to change anything about the new pass manager in this patch.

As to implementing print-after-all in the new pass manager, I do not know what should be the best way to do yet... Your idea seems to be a good solution.

I think it would be oddly divergent if these are analyses in one pass manager and not the other, which is why I'm pushing to understand what we'll do for both.

test/Other/printer.ll
11

Can you add a positive check that Print Module IR is actually being called?

tyb0807 updated this revision to Diff 153543.Jun 29 2018, 11:03 AM

Addressed review comment.

tyb0807 marked an inline comment as done.EditedJun 29 2018, 11:22 AM

I think it would be oddly divergent if these are analyses in one pass manager and not the other, which is why I'm pushing to understand what we'll do for both.

Hmmmm, in the new pass manager, it seems to me that passes are no longer inherited from Pass object, thus no longer classified as analyses or transformations (please correct me if I'm wrong)

Anyway, if this is not the right way to do, then we should change BitcodeWriter to a non-analysis pass I guess? There should be no difference between these 2 passes on whether it is an analysis or not IMHO

Is it better now?

dexonsmith accepted this revision.Jul 5 2018, 12:13 PM

Anyway, if this is not the right way to do, then we should change BitcodeWriter to a non-analysis pass I guess? There should be no difference between these 2 passes on whether it is an analysis or not IMHO

That's a good point. I'm fine with this then. LGTM.

This revision is now accepted and ready to land.Jul 5 2018, 12:13 PM

Thank you for reviewing this. Unfortunately I do not have the commit access, can you do it on my behalf please?
Thank you again

Friendly ping!

dexonsmith closed this revision.Jul 11 2018, 4:56 PM

Committed in r336869!