AffineApplyNormalizer::renumber returns invalid map

bondhugula commented 5 years ago

AffineApplyNormalizer::renumber isn't a publicly exposed method - it however generates invalid maps in some cases. Reproduce with two normalizers created with the maps below:

( )[s0] -> (s0) (%v1) ( )[s0] -> (s0) (%v2)

Calling renumber on the first with the second as argument yields: ( ) (s0) -> (s1) // invalid

The expressions it generates are correct, but the returned map's input dim/symbol count is incorrect (one instead of two here).

https://github.com/tensorflow/mlir/blob/d4e60ddaa853fd5954864a0165773314a8981de4/lib/AffineOps/AffineOps.cpp#L276

bondhugula commented 5 years ago

@nicolasvasilache I think the last line of AffineApplyNormalizer::renumber should have been

return map.replaceDimsAndSymbols(dimRemapping, symRemapping,
                                   reorderedDims.size(),
                                   concatenatedSymbols.size());

instead of

return map.replaceDimsAndSymbols(dimRemapping, symRemapping,
                                   dimRemapping.size(), symRemapping.size())

?

https://github.com/tensorflow/mlir/blob/d4e60ddaa853fd5954864a0165773314a8981de4/lib/AffineOps/AffineOps.cpp#L337

It doesn't affect any of the current test cases, but I think that's the bug here that may cause incorrect behavior if someone adds another helper method that reuses renumber.

nicolasvasilache commented 5 years ago

Thanks for your bug report, I am out till the middle of next week, I'll address when I come back.

On Fri, Aug 23, 2019, 12:40 PM Uday Bondhugula notifications@github.com wrote:

@nicolasvasilache https://github.com/nicolasvasilache I think the last line of AffineApplyNormalizer::renumber should have been

return map.replaceDimsAndSymbols(dimRemapping, symRemapping, reorderedDims.size(), concatenatedSymbols.size());

instead of

return map.replaceDimsAndSymbols(dimRemapping, symRemapping, dimRemapping.size(), symRemapping.size())

?

https://github.com/tensorflow/mlir/blob/d4e60ddaa853fd5954864a0165773314a8981de4/lib/AffineOps/AffineOps.cpp#L337

It doesn't affect any of the current test cases, but I think that's the bug here that may cause incorrect behavior if someone adds another helper method that reuses renumber.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tensorflow/mlir/issues/89?email_source=notifications&email_token=ACNNU5FSZM3U5ZF2ZJ7JCBTQGAHIZA5CNFSM4IMSSR5KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5AXFMY#issuecomment-524382899, or mute the thread https://github.com/notifications/unsubscribe-auth/ACNNU5CBCNWY2IGRV5YTMFLQGAHIZANCNFSM4IMSSR5A .

antiagainst commented 5 years ago

Oops, sorry my bad. Didn't notice @nicolasvasilache is OOO since this week. Reassigning to @andydavis1.

antiagainst commented 5 years ago

Somehow I cannot set @andydavis1 as the assignee. So just @ here.

tensorflow / mlir

AffineApplyNormalizer::renumber returns invalid map #89