Page MenuHomePhabricator

Simplify MachineVerifier's block-successor verification.
ClosedPublic

Authored by jyknight on May 12 2020, 9:57 AM.

Details

Summary

There's two properties we want to verify:

  1. That the successors returned by analyzeBranch are in the CFG successor list, and
  2. That there are no extraneous successors are in the CFG successor list.

The previous implementation mostly accomplished this, but in a very
convoluted manner.

Diff Detail

Event Timeline

jyknight created this revision.May 12 2020, 9:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2020, 9:57 AM
jyknight edited the summary of this revision. (Show Details)May 12 2020, 9:59 AM
arsenm accepted this revision.May 12 2020, 11:49 AM

LGTM with nits

llvm/lib/CodeGen/MachineVerifier.cpp
691

Braces

695

Braces

723

Capitalize

This revision is now accepted and ready to land.May 12 2020, 11:49 AM

Admittedly, I found this code to be a rats nest. Thanks for making it slightly neater. LGTM

llvm/lib/CodeGen/MachineVerifier.cpp
691

Why? The body is single statement.

708

It's weird seeing the bool && <sub expr that made up the bool>. Maybe

if (!Cond.empty() && !FBB) {

Then you could lower the declaration of Fallthrough closer to its use below?

arsenm added inline comments.May 12 2020, 12:22 PM
llvm/lib/CodeGen/MachineVerifier.cpp
691

Spanning multiple lines

aheejin added inline comments.
llvm/test/CodeGen/WebAssembly/eh-labels.mir
32

In this case, the next BB is an EH pad where we unwind to in case @foo throws, so the bb.2 becomes a fallthrough. Does this patch not handle this case?

jyknight marked an inline comment as done.May 15 2020, 1:00 PM
jyknight added inline comments.
llvm/test/CodeGen/WebAssembly/eh-labels.mir
32

Sorry, I don't follow. There's no magic behavior of "just skip over ehpad blocks and fallthrough into the next block after it."

bb.0 says it falls through into bb.2, but the instruction stream and block layout shows a fall-through into bb.1. That's now properly detected as invalid by the verifier, but was missed before.

Normally, blocks would be placed, so as not to have EHPad blocks in the middle of a potential fallthrough (e.g. moving bb.1 to the end of the function), but given the block placement specified in this test, there should be an explicit jump around the ehpad block.

jyknight updated this revision to Diff 264406.May 15 2020, 7:46 PM
jyknight marked 9 inline comments as done.

Fix nits.

llvm/lib/CodeGen/MachineVerifier.cpp
691

I'm pretty sure that's not an llvm style rule, so I'm going to leave it as is. All the existing code here is like that.

708

I like the first half of your suggestion, so done. But I think the description flows better to leave the definition of fallthrough where it is.

llvm/test/CodeGen/WebAssembly/eh-labels.mir
32

(Or alternatively, if @foo was a noreturn call, then bb.2 should not be marked as a successor of bb.0)

jyknight updated this revision to Diff 264407.May 15 2020, 7:50 PM

Oops, actually upload changed version. :)

nickdesaulniers accepted this revision.May 18 2020, 5:15 PM
This revision was automatically updated to reflect the committed changes.