Page MenuHomePhabricator

[MLIR] Flag no-terminator error on the last operation of non-empty blocks
ClosedPublic

Authored by jurahul on Nov 6 2020, 4:25 PM.

Details

Summary
  • When a block is not empty and does not end with a terminator, flag the error on the last operation of the block instead of the start of the block.

Diff Detail

Event Timeline

jurahul created this revision.Nov 6 2020, 4:25 PM
jurahul requested review of this revision.Nov 6 2020, 4:25 PM
mehdi_amini added inline comments.Nov 6 2020, 4:31 PM
mlir/test/IR/invalid.mlir
175

Can you provide a sample of the output before and after the code change on this example?

jurahul marked an inline comment as done.Nov 6 2020, 4:45 PM
jurahul added inline comments.
mlir/test/IR/invalid.mlir
175

The old example does not reproduce the issue. With the new example and no test MLIR changes, the error is flagged on @x

invalid.mlir split at line #171:4:8: error: block with no terminator
  %x = constant 0 : i32

With the change, its flagged on @y

invalid.mlir split at line #171:5:8: error: block with no terminator
  %y = constant 1 : i32

The last operation is not the most ideal, but much better than the first operation (of potentially long block) to indicate this error.

mehdi_amini accepted this revision.Nov 6 2020, 4:57 PM
mehdi_amini added inline comments.
mlir/test/IR/invalid.mlir
175

Yeah I mean the new example, thanks!

This revision is now accepted and ready to land.Nov 6 2020, 4:57 PM
rriddle accepted this revision.Nov 6 2020, 4:59 PM

FWIW I see this going either way. The first operation can often be a better location given that it is much closer to the when/where the block was created.

This revision was automatically updated to reflect the committed changes.
jurahul marked an inline comment as done.

This still seems confusing, since the operation itself may have a region with only one block. Maybe we could show a range of line numbers from the first and last instructions in the block?

I already push this in, but we can revisit this if there is a better way. Could you give an example where the confusion arises? To me, the last instruction seems more relevant because that's where something is missing. Are you suggesting doing:

if (block.back().isKnownNonTerminator()) {
  block.emitError("block with no terminator");
  return block.back().emitError("block with no terminator");
}

i.e., emit the error twice?