This is an archive of the discontinued LLVM Phabricator instance.

Avoid assertion failure on printing scf.for in debug dump.
Needs RevisionPublic

Authored by stellaraccident on Jun 3 2021, 10:35 PM.

Details

Reviewers
rriddle
Summary

I hit an assert about an scf.for having an empty region when attempting to debug via -convert-scf-to-std -debug. During conversion, scf.for ops can exist without a body and this makes it resilient to that.

Please advise if there is a way we test this kind of thing (or another preferred way to do it).

Diff Detail

Event Timeline

stellaraccident requested review of this revision.Jun 3 2021, 10:35 PM
rriddle added inline comments.Jun 3 2021, 11:36 PM
mlir/lib/Dialect/SCF/SCF.cpp
168

I'm not really pro doing this kind of thing (i.e. adding a bunch of ifs and checks in the pretty printers), IMO debug printing should generally go through the generic form.

rriddle added inline comments.Jun 3 2021, 11:57 PM
mlir/lib/Dialect/SCF/SCF.cpp
168

To clarify a bit further, you'll end up trying to duplicate the verifier in the pretty printer which isn't really practical.

rriddle requested changes to this revision.Jun 3 2021, 11:57 PM
This revision now requires changes to proceed.Jun 3 2021, 11:57 PM
jpienaar added inline comments.
mlir/lib/Dialect/SCF/SCF.cpp
168

Would it be possible to have debug always print the generic form? I don't know if we have a way of detecting in debug block and users can write arbitrary code ... And querying if debug is enabled on every print may be expensive (but that is only in non-opt builds)

stellaraccident added inline comments.Jun 4 2021, 9:03 AM
mlir/lib/Dialect/SCF/SCF.cpp
168

Yeah, I didn't expect this specific fix to be what we want to do, but we do need to do something: dialect conversion's debug printers currently crash on any pipeline that is doing SCFToStandard (and I've had other examples of it crashing on print), and we need that to be robust.

Should we change the debug prints in dialect conversion to generic form? Or find the case this is hitting and condition it to check the verifier first?

rriddle added inline comments.Jun 4 2021, 4:45 PM
mlir/lib/Dialect/SCF/SCF.cpp
168

I think dialect conversion should be printing in generic form (especially given that there are many ways that things can go wrong there).

One thing that has been mentioned in the past, is trying to run the verifier before printing to see which printer to use (but that is a semi-unrelated discussion to here).

stellaraccident added inline comments.Jun 4 2021, 7:08 PM
mlir/lib/Dialect/SCF/SCF.cpp
168

Agreed - I get a lot of crashes when using -debug with dialect conversion on various op sets. I like the custom printing but not at the expense of getting a debug dump.

I'll update the patch to change dialect conversion to print generic, unless if you have a preference for taking it.

mehdi_amini added inline comments.Jun 4 2021, 7:35 PM
mlir/lib/Dialect/SCF/SCF.cpp
168

One thing that has been mentioned in the past, is trying to run the verifier before printing to see which printer to use

We do that from Python when printing :)