Page MenuHomePhabricator

[mlir] Fix scf.for single iteration canonicalization check
ClosedPublic

Authored by antiagainst on Jan 26 2021, 6:18 AM.

Details

Summary

We should be check whether lb + step >= ub to determine
whether this is a single iteration. Previously we were
checking lb + lb >= ub.

Diff Detail

Event Timeline

antiagainst created this revision.Jan 26 2021, 6:18 AM
antiagainst requested review of this revision.Jan 26 2021, 6:18 AM

Nit: could you add bug description in change description?

ftynse requested changes to this revision.Jan 26 2021, 6:42 AM
ftynse added inline comments.
mlir/test/Dialect/Linalg/fusion.mlir
668

Why is this change necessary?

mlir/test/Dialect/SCF/canonicalize.mlir
240

I'd rather have a new test that exercises the previously wrong behavior. This doesn't seem to do so precisely because it is changed to have step value == upper bound value.

mlir/test/mlir-cpu-runner/mlir_test_cblas_interface.cpp
74–92 ↗(On Diff #319280)

This looks irrelevant

This revision now requires changes to proceed.Jan 26 2021, 6:42 AM

Address comments

mlir/test/Dialect/Linalg/fusion.mlir
668

Because otherwise one scf.for loop will be canonicalized away. I think the purpose of this test is to check the two scf.for and linalg.fill/linalg.conv nested inside of them. So without the canonicalization it makes things clearer. Also, most of the tests in this file is performing on unknown dims.

mlir/test/Dialect/SCF/canonicalize.mlir
240

I've created a new test for this and made the step != lb & step != ub. :)

mlir/test/mlir-cpu-runner/mlir_test_cblas_interface.cpp
74–92 ↗(On Diff #319280)

It was needed because of the scf.for canonicalization kicked in so some shapes are not ? anymore. But Nicolas killed this entire file. :)

antiagainst retitled this revision from [mlir] Fix scf.for single iteration canonicalization bug to [mlir] Fix scf.for single iteration canonicalization check.Feb 2 2021, 5:02 AM
antiagainst edited the summary of this revision. (Show Details)

Nit: could you add bug description in change description?

Done.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 2 2021, 8:12 AM
This revision was automatically updated to reflect the committed changes.
antiagainst reopened this revision.Feb 2 2021, 8:15 AM

Sorry I accidentally landed this together with a following patch (which actually does not have dependency).. Reverted.

ftynse accepted this revision.Feb 2 2021, 11:02 AM
This revision is now accepted and ready to land.Feb 2 2021, 11:02 AM