Page MenuHomePhabricator

[AsmPrinter] Remove hidden flag -print-schedule.
ClosedPublic

Authored by andreadb on Jan 25 2019, 9:00 AM.

Details

Summary

This patch removes hidden codegen flag -print-schedule effectively reverting the logic originally committed as r300311 (https://llvm.org/viewvc/llvm-project?view=revision&revision=300311).

Flag -print-schedule was originally introduced by r300311 to address PR32216 (https://bugs.llvm.org/show_bug.cgi?id=32216). That bug was about adding "Better testing of schedule model instruction latencies/throughputs".

These days, we can use llvm-mca to test scheduling models. So there is no longer a need for flag -print-schedule in LLVM. The main use case for PR32216 is addressed by llvm-mca.
Flag -print-schedule is mainly used for debugging purposes, and it is only actually used by x86 specific tests. We already have extensive (latency and throughput) tests under "test/tools/llvm-mca" for X86 processor models. That means, most (if not all) existing -print-schedule tests for X86 are redundant.

When flag -print-schedule was first added to LLVM, several files had to be modified; a few APIs gained new arguments (see for example method MCAsmStreamer::EmitInstruction), and MCSubtargetInfo/TargetSubtargetInfo gained a couple of getSchedInfoStr() methods.

Method getSchedInfoStr() had to originally work for both MCInst and MachineInstr. The original implmentation of getSchedInfoStr() introduced a subtle layering violation (reported as PR37160 and then fixed/worked-around by r330615).
In retrospect, that new API could have been designed more optimally. We can always query MCSchedModel to get the latency and throughput. More importantly, the "sched-info" string should not have been generated by the subtarget.
Note, r317782 fixed an issue where "print-schedule" didn't work very well in the presence of inline assembly. That commit is also reverted by this change.

In conclusion: I think that we can safely remove the -print-schedule flag. It was previously very useful despite the above issues because there was no better alternative. Future tests for machine schedulers can use llvm-mca as tool.

Apologies for the large patch; most of the difference is because several tests have been removed. The rest of the diff is a clean-up, and lines have been removed.
I didn't remove the avx512 tests mainly because I am not entirely sure if we have extensive coverage for those in llvm-mca. For now, I have simply regenerated those using update_llc.py (after having removed flag -print-schedule).

Please let me know what you think.

Thanks in advance,
Andrea

Diff Detail

Repository
rL LLVM

Event Timeline

andreadb created this revision.Jan 25 2019, 9:00 AM

avx512vpopcntdq-schedule.ll can be deleted - its fully covered in llvm-mca tests

avx512-shuffle-schedule.ll is covered by llvm-mca tests - I don't think it gives us much in additional codegen test coverage, we can maybe keep it around as avx512-shuffle.ll ?

avx512-schedule.ll - this isn't fully covered by llvm-mca tests, it also has some tests that don't look like they belong there (_mm512_set_epi32, stack folding tests etc.)

@craig.topper what do you think?

How much of avx512-schedule.ll isn't covered by llvm-mca? The test itself seems to be a lot of copy and pastes from other files. I don't know if there's any unique content in it.

How much of avx512-schedule.ll isn't covered by llvm-mca? The test itself seems to be a lot of copy and pastes from other files. I don't know if there's any unique content in it.

I'll try to go through the file and audit it, I think its mainly avx512dq instructions that aren't covered by llvm-mca tests. And I'll move the non-schedule tests into somewhere more suitable.

RKSimon added a comment.EditedJan 26 2019, 5:26 AM

I've gone through avx512-schedule.ll - the tests are duplicates from various other avx512 test files, sometimes renamed, to add schedule test coverage - imo the file can be deleted.

I've done a first pass of adding the llvm-mca tests at rL352273, we need to go through and do a more thorough pass at some point to ensure we have full ISA/schedule-model coverage but I don't think there's anything stopping us getting rid of the -schedule.ll files at this point.

andreadb updated this revision to Diff 183835.Jan 28 2019, 5:47 AM

Patch updated.

Removed avx512 schedule tests (thanks Simon!).

This makes sense to me - tbh I'm not sure if we gain anything from keeping -print-schedule any more - does anyone have any further comments?

test/CodeGen/X86/mul-constant-i32.ll
9 ↗(On Diff #183835)

Please add common check-prefixes to collapse this more (same for mul-constant-i64.ll).

RKSimon accepted this revision.Jan 31 2019, 8:24 AM

Looks like there's no objections, so LGTM!

This revision is now accepted and ready to land.Jan 31 2019, 8:24 AM
Closed by commit rL353043: [AsmPrinter] Remove hidden flag -print-schedule. (authored by adibiagio, committed by ). · Explain WhyFeb 4 2019, 4:51 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2019, 4:51 AM
Herald added a subscriber: jrtc27. · View Herald Transcript
bcain added a subscriber: bcain.Feb 26 2019, 7:04 AM