- 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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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.
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? |
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.
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.
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.
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.
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. |
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. |
test/CodeGen/MIR/X86/auto-successor.mir | ||
---|---|---|
1 ↗ | (On Diff #97765) | I meant in the same file. |