Page MenuHomePhabricator

MIParser/MIRPrinter: Compute block successors if not explicitely specified
ClosedPublic

Authored by MatzeB on Mar 22 2017, 2:39 PM.

Details

Summary
  • MIParser: If the successor list is not specified successors will be added based on basic block operands in the block and possible fallthrough.
  • MIRPrinter: Skip printing of block successor lists in cases where the parser is guaranteed to reconstruct it. This means we still print the list if some successor cannot be determined (happens for example for jump tables), if the successor order changes or branch probabilities being unequal.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB created this revision.Mar 22 2017, 2:39 PM
MatzeB updated this revision to Diff 92724.Mar 22 2017, 3:26 PM

Updated:

  • Actually remove nearly all the successors: lines from X86,ARM,AArch64 and AMDGPU tests
  • Ignore phi block arguments now
  • Fix a bug when condjump and fallthrough to the same block corner case: We would get the same successor twice instead of just one which MI expects.
qcolombet edited edge metadata.Mar 24 2017, 9:12 PM

Hi Matthias,

Thanks for doing this, this is a long overdue!

MIRPrinter: Skip printing of block successor lists in cases where the parser is guaranteed to reconstruct it.

I'd like the printer to always print everything. I don't think there is any value in having magic here.

Therefore, LGTM modulo drop the test case changes and MIRPrinter changes.

Cheers,
-Quentin

lib/CodeGen/MIRParser/MIParser.cpp
16 ↗(On Diff #92724)

Do we have other relative path in the code base?
It feels awful.

ab edited edge metadata.Mar 24 2017, 10:13 PM

I'd like the printer to always print everything. I don't think there is any value in having magic here.

IMO this kind of magic is very useful: it makes output much easier to read, and easier to start from when writing tests.

MatzeB added a comment.Apr 4 2017, 1:45 PM
In D31262#710545, @ab wrote:

I'd like the printer to always print everything. I don't think there is any value in having magic here.

IMO this kind of magic is very useful: it makes output much easier to read, and easier to start from when writing tests.

I also think there is value in reducing the amount of information printed. In this specific case the information omitted is quite obvious IMO.

MatzeB added inline comments.Apr 4 2017, 1:49 PM
lib/CodeGen/MIRParser/MIParser.cpp
16 ↗(On Diff #92724)

Not many, but there are some in ExecutionEngine/RuntimeDyld, for example:

ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFI386.h:#include "../RuntimeDyldCOFF.h"

but ok, I can move MIRPrinter.h to include/llvm/CodeGen instead. I'll do that in the next update.

I also think there is value in reducing the amount of information printed. In this specific case the information omitted is quite obvious IMO.

Something we talked with Vivek, if we really want something easier to edit, we could have two modes:

  • One that prints everything (the default)
  • One that prints only what cannot be deduced and is meant for editing

I really believe keeping the printer simple (print everything) for the default case is more sustainable in the long run.

Cheers,
-Quentin

In a nutshell, I am saying that we shouldn't conflict two goals: editing and printing. Printing should be full fledged, editing should be minimal.

MatzeB added a comment.May 1 2017, 5:03 PM

In a nutshell, I am saying that we shouldn't conflict two goals: editing and printing. Printing should be full fledged, editing should be minimal.

Sounds like we are not in sync what we do with .mir... It is a tool to create llvm lit tests, isn't it? Printing at least today is mostly done via MachineFunction::print() etc. Sure we want to unify the formats used there and .mir in the long term. But printing with llc -stop-after is the main tool to get started writing a lit test and we should promoted/encourage nice syntax there.

MatzeB updated this revision to Diff 97764.May 3 2017, 6:47 PM

I'm still not completely convinced. But in the interest of compromise and getting this patch through:

  • Added a -simplify-mir flag (off by default) that controls whether unnecessary successor list will be omitted.
MatzeB updated this revision to Diff 97765.May 3 2017, 6:50 PM
  • Move MIRPrinter.h to include/llvm/CodeGen to avoid ../ relative path.
qcolombet accepted this revision.May 4 2017, 10:49 AM

Thanks Matthias.

LGTM, nitpicks below.

include/llvm/CodeGen/MIRPrinter.h
42 ↗(On Diff #97765)

Although I understand why this is here, this illustrates my point that this should be a parser thing.

test/CodeGen/MIR/X86/auto-successor.mir
1 ↗(On Diff #97765)

Could we have a positive test as well? (I.e., where successors are printed.)

test/CodeGen/MIR/X86/branch-probabilities.mir
6 ↗(On Diff #97765)

This does not seem related to this patch.

This revision is now accepted and ready to land.May 4 2017, 10:49 AM
MatzeB marked 2 inline comments as done.May 5 2017, 2:16 PM

Thanks for the review.

test/CodeGen/MIR/X86/auto-successor.mir
1 ↗(On Diff #97765)

bb.0, bb.1, bb.2 are various levels of positive tests. For printing without -simplify-mir we already have test/CodeGen/MIR/X86/successor-basic-blocks.mir.

test/CodeGen/MIR/X86/branch-probabilities.mir
6 ↗(On Diff #97765)

This is a translation of test/CodeGen/MIR/Generic/branch-probabilities.ll to .mir.

When this patch had the -simplify-mir beahvior the existing test would break because the probabilities were just balanced and therefore perfectly predictable.

Even though the change is not necessary anymore as the default printing behavior doesn't change I think I'll still keep this in here as it improves upon the robustness of the test.

This revision was automatically updated to reflect the committed changes.
qcolombet added inline comments.May 8 2017, 8:06 AM
test/CodeGen/MIR/X86/auto-successor.mir
1 ↗(On Diff #97765)

I meant in the same file.
Like having this test being run twice.