This is an archive of the discontinued LLVM Phabricator instance.

[mlir][scf] Fix a bug in scf::ForOp loop unroll with an epilogue
ClosedPublic

Authored by tatianashp on Sep 12 2020, 7:22 PM.

Details

Summary

Fixes a bug in formation and simplification of an epilogue loop generated
during loop unroll of scf::ForOp (https://bugs.llvm.org/show_bug.cgi?id=46689)

Diff Detail

Event Timeline

tatianashp created this revision.Sep 12 2020, 7:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 12 2020, 7:22 PM
tatianashp requested review of this revision.Sep 12 2020, 7:22 PM
mehdi_amini accepted this revision.Sep 12 2020, 9:30 PM
mehdi_amini added inline comments.
mlir/test/Transforms/scf-loop-unroll.mlir
2

Can you remove the -allow-unregistered-dialect here and use std.add (or another test operation), and same for types?

This revision is now accepted and ready to land.Sep 12 2020, 9:30 PM
tatianashp edited the summary of this revision. (Show Details)Sep 13 2020, 12:57 PM
bondhugula requested changes to this revision.Sep 13 2020, 11:09 PM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/lib/Transforms/Utils/LoopUtils.cpp
217

Nit: consider using getTerminator() instead of back() for better readability.

mlir/test/Transforms/scf-loop-unroll.mlir
9

Could you make it 11 iterations to make the test stronger (or an additional one if you prefer)? Is coverage missing for a symbolic upper bound in the presence of return values?

This revision now requires changes to proceed.Sep 13 2020, 11:09 PM
mehdi_amini added inline comments.Sep 13 2020, 11:13 PM
mlir/test/Transforms/scf-loop-unroll.mlir
9

I believe the crash from PR46689 only exists when the epilogue has a single iteration

Update: Fix handling of loop carried variables inside the loop.
This update addresses reviewer comments and fixes another, previously undetected bug:

  • Loop carried variables are now correctly propagated during unrolling the body of the loop.
  • Added another test with symbolic upper bound and multiple loop-carried variables.
tatianashp marked 2 inline comments as done.Oct 4 2020, 10:30 AM
tatianashp added inline comments.
mlir/test/Transforms/scf-loop-unroll.mlir
9

I added another test with a symbolic upper bound and multiple carried values to test the case when epilogue loop is not unrolled. It does not trigger crash from PR46689, but good to have as additional coverage.

tatianashp marked 2 inline comments as done.Oct 4 2020, 10:30 AM

Seems like you uploaded the same diff as before, you may want to double-check and retry here?

tatianashp updated this revision to Diff 296058.EditedOct 4 2020, 10:51 AM

Uploaded changes.

Remove changes pulled in from unrelated updates to mlir/lib/Transforms/Utils/LoopUtils.cpp

mehdi_amini accepted this revision.Oct 4 2020, 3:16 PM

LGTM again, thanks. You probably should wait one or two days to give a change to Uday to check again.

bondhugula accepted this revision.Oct 4 2020, 11:26 PM
bondhugula added inline comments.
mlir/lib/Transforms/Utils/LoopUtils.cpp
490–494

Nit: These can be made triple /// as well while at this.

520

Nit: please use i = 0, e = ...; i < e form.

580

Nit: /*iterArgs=*/{}, /*yieldedValues=*/{};

This revision is now accepted and ready to land.Oct 4 2020, 11:26 PM
tatianashp marked 2 inline comments as done.

A few minor changes addressing Uday's comments.

Mehdi, Uday - would you like to commit this diff? I don't have commit priveledges.

Mehdi, Uday - would you like to commit this diff? I don't have commit priveledges.

Sure! But it needs to be rebased right now I believe.

bondhugula accepted this revision.Oct 10 2020, 1:44 AM