This is an archive of the discontinued LLVM Phabricator instance.

Add test coverage for new PM PGOOpt handling.
ClosedPublic

Authored by danielcdh on Jul 24 2017, 10:32 AM.

Details

Summary

This patch adds flags and tests to cover the PGOOpt handling logic in new PM.

Event Timeline

danielcdh created this revision.Jul 24 2017, 10:32 AM
davide requested changes to this revision.Jul 24 2017, 11:28 AM

This is a step in the right direction, some comments inline.

lib/Passes/PassBuilder.cpp
1109 ↗(On Diff #107927)

s/Use/Process/ ?

1116 ↗(On Diff #107927)

Do you need return after fatal?

1118 ↗(On Diff #107927)

Capital P

1120–1135 ↗(On Diff #107927)

StringSwitch maybe?

This revision now requires changes to proceed.Jul 24 2017, 11:28 AM
danielcdh edited edge metadata.
danielcdh marked 4 inline comments as done.

address David's comments.

chandlerc edited edge metadata.Jul 25 2017, 3:39 AM

Awesome, I'm glad to see a nice way of testing this at a low level!

lib/Passes/PassBuilder.cpp
175–178 ↗(On Diff #107939)

Rather than these options going in PassBuilder.cpp and overriding things, I think this should go into tools/opt/NewPMDriver.cpp much like the ThinLTO controls do.

Then the opt tool can construct a PGOOpt struct and hand it into the PassBuilder much the same way Clang will.

Also, sample_gen doesn't appear to be valid?

I think rather than having one flag that you have to parse I would suggest have three flags: pgo-instr-gen, pgo-intsr-use, and pgo-sample-use. I think there is a way in the cl::opt layer to say the flags are mutually incompatible, but even if you have to do that manually, it seems better than parsing an '=' out of the flag value.

danielcdh updated this revision to Diff 108167.Jul 25 2017, 3:27 PM
danielcdh marked an inline comment as done.

update

danielcdh marked an inline comment as done.Jul 25 2017, 3:27 PM
danielcdh added inline comments.
lib/Passes/PassBuilder.cpp
175–178 ↗(On Diff #107939)

pgo-instr-{gen|use}, conflict with existing flags. Used pgo-{gen|use|sample-use} instead.

Minor flag tweak...

tools/opt/NewPMDriver.cpp
169–185

What about a simpler factoring of flags:

cl::opt<PGOKind> PGOKindFlag(
    cl::values(clEnumValN(PGOKind::InstrGen, "new-pm-pgo-instr-gen-pipeline", "..."),
               clEnumValN(PGOKind::InstrUse, "new-pm-pgo-instr-use-pipeline", "..."),
               clEnumValN(PGOKind::SampleUse, "new-pm-pgo-sample-use-pipeline", "...")));

cl::opt<std::string> ProfileFile(...);

This would do the checking that only one kind is selected at a time for you I think.

danielcdh updated this revision to Diff 108199.Jul 25 2017, 6:38 PM
danielcdh marked an inline comment as done.

update

danielcdh marked an inline comment as done.Jul 25 2017, 6:38 PM
chandlerc accepted this revision.Jul 25 2017, 6:54 PM

I think the command line flags can be made simpler, but I must not be explaining it well in review. I suggest you go ahead and submit this and I'll immediately send you a follow-up patch that hopefully illustrates what I'm suggesting for handling the flags. And that will let you rebase and continue on other things.

I think the command line flags can be made simpler, but I must not be explaining it well in review. I suggest you go ahead and submit this and I'll immediately send you a follow-up patch that hopefully illustrates what I'm suggesting for handling the flags. And that will let you rebase and continue on other things.

Thanks, will do now.

This revision was automatically updated to reflect the committed changes.