This is an archive of the discontinued LLVM Phabricator instance.

[mlir][OpDSL] Fix OpDSL tests after https://reviews.llvm.org/D114680.
ClosedPublic

Authored by gysit on Nov 29 2021, 11:56 AM.

Details

Summary

Update the shapes of the convolution / pooling tests that where detected after enabling verification during printing (https://reviews.llvm.org/D114680). Also split the emit_structured_generic.py file that previously contained all tests into multiple separate files to simplify debugging.

Diff Detail

Event Timeline

gysit created this revision.Nov 29 2021, 11:56 AM
gysit requested review of this revision.Nov 29 2021, 11:56 AM

Splitting and fixing at the same time kills the diff, it'd be better to have this in two steps otherwise I'm not sure how this is reviewable.

Also your revision says it is fixing the test, but the bot is passing right now: https://lab.llvm.org/buildbot/#/builders/61/builds/18113 ; are they not running in CI? What's missing?

Splitting and fixing at the same time kills the diff, it'd be better to have this in two steps otherwise I'm not sure how this is reviewable.

Also your revision says it is fixing the test, but the bot is passing right now: https://lab.llvm.org/buildbot/#/builders/61/builds/18113 ; are they not running in CI? What's missing?

As part of reworking it, Tobias removed the TODO/revert to old behavior I added:

# TODO: Fix me! Conv and pooling ops above do not verify, which was uncovered
# when switching to more robust module verification. For now, reverting to the
# old behavior which does not verify on module print.
print(module.operation.get_asm(assume_verified=True))

So this is fixing the test such that it verifies properly.

I agree that it would have been better to have staged this for a better diff/review. In this case, though, I'm familiar enough that I can take the effort to review as-is: maybe consider Mehdi's advice for the future.

stellaraccident accepted this revision.Nov 29 2021, 1:29 PM

Thanks for the fix (by way of fixing the shapes so that we could remove the fallback to unverified printing). I was able to line things up on my screen to side by side review, but for the future, as Mehdi says: an NFC split followed by a fix to the few impacted test cases would be better (or the other way around).

mlir/test/python/dialects/linalg/opdsl/emit_pooling.py
73

Nit: Pooling indexing maps.

This revision is now accepted and ready to land.Nov 29 2021, 1:29 PM

Splitting and fixing at the same time kills the diff, it'd be better to have this in two steps otherwise I'm not sure how this is reviewable.

Also your revision says it is fixing the test, but the bot is passing right now: https://lab.llvm.org/buildbot/#/builders/61/builds/18113 ; are they not running in CI? What's missing?

As part of reworking it, Tobias removed the TODO/revert to old behavior I added:

Ah got it! Thanks :)

gysit updated this revision to Diff 390603.Nov 30 2021, 12:42 AM

Address comment.

gysit marked an inline comment as done.Nov 30 2021, 12:49 AM

Good point! Still working on making my patches smaller and one thing at the time only...

This revision was landed with ongoing or failed builds.Nov 30 2021, 12:58 AM
This revision was automatically updated to reflect the committed changes.
mlir/test/python/dialects/linalg/opdsl/emit_pooling.py