Page MenuHomePhabricator

[llc] (almost) remove `--print-machineinstrs`
ClosedPublic

Authored by ychen on Jul 6 2020, 8:40 PM.

Details

Summary

Its effect could be achieved by
-stop-after,-print-after,-print-after-all. But a few tests need to
print MIR after ISel which could not be done with
-print-after/-stop-after since isel pass does not have commandline name.
That's the reason --print-machineinstrs is downgraded to
--print-after-isel in this patch. --print-after-isel could be
removed after we switch to new pass manager since isel pass would have a
commandline text name to use print-after or equivalent switches.

The motivation of this patch is to reduce tests dependency on
would-be-deprecated feature.

Diff Detail

Event Timeline

ychen created this revision.Jul 6 2020, 8:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2020, 8:40 PM
dsanders accepted this revision.Jul 6 2020, 9:36 PM

I was worried for a moment that we'd be losing the ability to print between all machine passes but it looks like -print-after-all covers that now (I don't think that was always the case). So long as we aren't losing any inter-pass dumps this LGTM but I'd suggest giving it a few days to see if anyone was using this in a way that isn't evident from the tests.

-print-after/-stop-after since isel pass does not have commandline name.

IIRC, there's something weird going on in this area. I vaguely remember a problem I never got to the bottom of where there was no name when AMDGPU was omitted but when it was compiled, everybody's pass was called amdgpu-isel. It had something to do with AMDGPU needing additional dependencies and using INITIALIZE_PASS to get them.

llvm/lib/CodeGen/TargetPassConfig.cpp
144–148

There's a typo (:->') but also there's more tests affected than just the two. Probably best to keep the exact number vague

This revision is now accepted and ready to land.Jul 6 2020, 9:36 PM
ychen added a comment.Jul 6 2020, 10:05 PM

I was worried for a moment that we'd be losing the ability to print between all machine passes but it looks like -print-after-all covers that now (I don't think that was always the case). So long as we aren't losing any inter-pass dumps this LGTM but I'd suggest giving it a few days to see if anyone was using this in a way that isn't evident from the tests.

-print-after/-stop-after since isel pass does not have commandline name.

IIRC, there's something weird going on in this area. I vaguely remember a problem I never got to the bottom of where there was no name when AMDGPU was omitted but when it was compiled, everybody's pass was called amdgpu-isel. It had something to do with AMDGPU needing additional dependencies and using INITIALIZE_PASS to get them.

I don't know if it is the cause but AMDGPUDAGToDAGISel::ID is actually SelectionDAGISel::ID.

arsenm accepted this revision.Jul 7 2020, 6:08 AM

I've never used -print-machineinstrs and was surprised it existed last time I noticed it

llvm/test/CodeGen/Hexagon/ifcvt-edge-weight.ll
1

This test should probably be switched to stop-after and MIR

llvm/test/DebugInfo/WebAssembly/dbg-value-move-2.ll
1–2

Ditto

llvm/test/DebugInfo/WebAssembly/dbg-value-move.ll
1–2

Ditto

I was worried for a moment that we'd be losing the ability to print between all machine passes but it looks like -print-after-all covers that now (I don't think that was always the case). So long as we aren't losing any inter-pass dumps this LGTM but I'd suggest giving it a few days to see if anyone was using this in a way that isn't evident from the tests.

-print-after/-stop-after since isel pass does not have commandline name.

IIRC, there's something weird going on in this area. I vaguely remember a problem I never got to the bottom of where there was no name when AMDGPU was omitted but when it was compiled, everybody's pass was called amdgpu-isel. It had something to do with AMDGPU needing additional dependencies and using INITIALIZE_PASS to get them.

I don't know if it is the cause but AMDGPUDAGToDAGISel::ID is actually SelectionDAGISel::ID.

That could be it. Those ID's are the keys into the pass registry so that would cause AMDGPUDAGToDAGISel to register a PassInfo for SelectionDAGISel with the argument "amdgpu-isel" which would be how other targets are able to use it. Without it compiled in, there would be no PassInfo registered leaving getPassIDFromName() unable to find it.

ychen marked an inline comment as done.Jul 17 2020, 3:00 PM
ychen added inline comments.
llvm/test/CodeGen/Hexagon/ifcvt-edge-weight.ll
1

There is a general problem is that -printer-after uses MachineFunctionPrinterPass(MachineFunction::print) whereas -stop-after uses MIRPrinter(in MIR serialization format). The general question is could we just use only of them? thoughts? maybe MIRPrinter since it *seems* the information it prints is a superset of what MachineFunction::print prints.

For this particular test, I could use stop-after because the check lines are simple. For the next two, using stop-after makes the tests fail. How about I add FIXME instead?

arsenm added inline comments.Jul 17 2020, 3:03 PM
llvm/test/CodeGen/Hexagon/ifcvt-edge-weight.ll
1

Yes, the output of the machine debug dumps and MIR dumps are slightly different. They're gradually converging. Eventually we should probably only have the MIR dumps, with some simplifications for debug printing cases (e.g. prune some of the function metadata in the common cases)

I think there's no legitimate reason for a test to be scraping the machine debug dumps when it could be checking the MIR instead.

arsenm added inline comments.Jul 17 2020, 3:05 PM
llvm/test/CodeGen/Hexagon/ifcvt-edge-weight.ll
1

Why do they fail? The "After.." checks can just be deleted.

There may may be a complication for the DBG_VALUE, I think there are some issues with serialization of debug info in MIR still

ychen marked 2 inline comments as done.Jul 17 2020, 3:06 PM
ychen added inline comments.
llvm/test/CodeGen/Hexagon/ifcvt-edge-weight.ll
1

Yes, for dbg-value-move-2.ll, fib2.c:6:7 is checked.

arsenm added inline comments.Jul 17 2020, 3:12 PM
llvm/test/CodeGen/Hexagon/ifcvt-edge-weight.ll
1

Yes, this is just a MIR printer bug

ychen updated this revision to Diff 278912.Jul 17 2020, 3:19 PM
  • add FIXME
  • fix typo
arsenm added inline comments.Jul 17 2020, 3:51 PM
llvm/lib/CodeGen/TargetPassConfig.cpp
144–148

Or GlobalISel, whichever gets there first...

llvm/test/CodeGen/ARM/taildup-branch-weight.ll
0–1

Most of these still look like they can easily be replaced with stop-after

ychen updated this revision to Diff 279062.EditedJul 18 2020, 11:42 PM
  • use -stop-after where possible
  • update comment
arsenm accepted this revision.Jul 20 2020, 9:54 AM

LGTM

This revision was automatically updated to reflect the committed changes.