This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Arith] Make previous load-bearing assert into a real error
ClosedPublic

Authored by krzysz00 on Jul 12 2023, 8:05 AM.

Details

Summary

When I landed the EmulateUnsupportedFloats, I'd negligently included
an assert that needed to run for the pass to be correct. Previous
emergency fix commits removed the assert. This commit re-adds the
"can't happen" testing as an emitOpError() and aborting the rewrite,
thus allowing it to function in no-assertions builds.

Diff Detail

Event Timeline

krzysz00 created this revision.Jul 12 2023, 8:05 AM
Herald added a project: Restricted Project. · View Herald Transcript
krzysz00 requested review of this revision.Jul 12 2023, 8:05 AM
kuhar accepted this revision.Jul 12 2023, 1:08 PM
kuhar added inline comments.
mlir/lib/Dialect/Arith/Transforms/EmulateUnsupportedFloats.cpp
85–91

uber-nit: Do we need this local variable? This is fine, but I'm more used to seeing:

if (failed(converter->convertTypes(op->getResultTypes(), resultTypes)))
  ...
89

nit: I don't think the part after the comma adds much, I'd remove it

This revision is now accepted and ready to land.Jul 12 2023, 1:08 PM
krzysz00 updated this revision to Diff 539722.Jul 12 2023, 2:05 PM

Address review nits