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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
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. |
Good point! Still working on making my patches smaller and one thing at the time only...
Nit: Pooling indexing maps.