This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Affine] Fix vectorizability check for multiple load/stores
ClosedPublic

Authored by sgrechanik on Dec 4 2020, 3:29 PM.

Details

Summary

This patch fixes a bug that allowed vectorizing of loops with loads and
stores having indexing functions varying along different memory
dimensions.

Diff Detail

Event Timeline

sgrechanik created this revision.Dec 4 2020, 3:29 PM
sgrechanik requested review of this revision.Dec 4 2020, 3:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 4 2020, 3:29 PM
aartbik added inline comments.Dec 7 2020, 9:56 AM
mlir/lib/Analysis/LoopAnalysis.cpp
343

note that we have a subtle difference of not setting memRefDim for -1 after this change;
that is probably harmless, but perhaps you want this out of the if to preserve behavior?

mlir/test/Dialect/Affine/SuperVectorize/vectorize_1d.mlir
402

can you please indent the CHECKs that are inside the method body?

sgrechanik updated this revision to Diff 310042.Dec 7 2020, 3:40 PM
sgrechanik marked an inline comment as done.
  • Set *memRefDim to -1 in the beginning
  • Fix indentation for the test case
mlir/lib/Analysis/LoopAnalysis.cpp
343

I think that if we have some memory ops that are invariant along the induction variable and some ops that vary along the same memory dimension N, then we should write N to *memRefDim. So we cannot write -1 to *memRefDim if isContiguousAccess returned -1, otherwise we may overwrite N.
However we should probably set *memRefDim to -1 at the beginning of this function in case the caller didn't do this.

Thanks for fixing this, Sergei! LGTM. I'll leave the final approval to Aart or Nicolas.

aartbik accepted this revision.Dec 8 2020, 2:26 PM
This revision is now accepted and ready to land.Dec 8 2020, 2:26 PM
This revision was landed with ongoing or failed builds.Dec 9 2020, 12:38 PM
This revision was automatically updated to reflect the committed changes.