Page MenuHomePhabricator

Fix verifier crashing on some invalid IR
ClosedPublic

Authored by mehdi_amini on Jun 16 2021, 12:03 PM.

Details

Summary

In a region with multiple blocks the verifier will try to look for
dominance and may get successor list for blocks, even though a block
may be empty or does not end with a terminator.

Diff Detail

Event Timeline

mehdi_amini created this revision.Jun 16 2021, 12:03 PM
mehdi_amini requested review of this revision.Jun 16 2021, 12:03 PM
rriddle accepted this revision.Jun 16 2021, 12:11 PM
rriddle added inline comments.
mlir/lib/IR/Block.cpp
349

block->empty is redundant with the llvm::hasSingleElement check.

This revision is now accepted and ready to land.Jun 16 2021, 12:11 PM
rriddle added inline comments.Jun 16 2021, 12:12 PM
mlir/lib/IR/Block.cpp
349

Sorry, meant to drop this comment after I actually looked at the commit (with non-tired eyes).

GMNGeoffrey added inline comments.Jun 16 2021, 12:27 PM
mlir/test/IR/invalid.mlir
181–187

Wait this is all that was needed to repro? How come I wasn't able to extract mhlo.sort from my repro? I thought it required an op with a region that wasn't isolated from above

mehdi_amini added inline comments.Jun 16 2021, 12:36 PM
mlir/test/IR/invalid.mlir
181–187

You sent me two bugs, this is the "small" repro.

The larger repro can't be test with opt like that, I would need to write a pass that intentionally create invalid IR and calls the verifier on a nested operation to setup exactly what you have. But the fix here will be enough to cover both test cases.

This revision was landed with ongoing or failed builds.Jun 16 2021, 12:36 PM
This revision was automatically updated to reflect the committed changes.

Thanks!

mlir/test/IR/invalid.mlir
181–187

Yeah I wasn't able to drop the mhlo.sort from even the smaller repro, I thought, but probably I was just doing it wrong :-)

Interesting!

mlir/lib/IR/Block.cpp
349

Nit: *Might* be better more readable with:

 if (block->empty() || llvm::hasSingleElement(*block->getParent()))
  return;
...
mehdi_amini added inline comments.Jun 16 2021, 12:43 PM
mlir/lib/IR/Block.cpp
349

Done in 066b3207234d