This is an archive of the discontinued LLVM Phabricator instance.

[mlir][AffineMap] NFC - Refactor getProjectedMap and split into projectDims and projectSymbols
ClosedPublic

Authored by nicolasvasilache on Mar 22 2023, 2:22 AM.

Details

Summary

The default behavior of getProjectedMap may be surprising as it implicitly compresses the dims and
the unused symbols.

Make these explicit in the API and refactor to more idiomatic implementations with better reuse.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald Transcript
nicolasvasilache requested review of this revision.Mar 22 2023, 2:22 AM
Herald added a project: Restricted Project. · View Herald Transcript
bondhugula added inline comments.Mar 22 2023, 2:52 AM
mlir/include/mlir/IR/AffineMap.h
575–576

Please avoid Flag suffixes; it's redundant and inconsistent with the style here.

qcolombet accepted this revision.Mar 22 2023, 2:59 AM
qcolombet added inline comments.
mlir/include/mlir/IR/AffineMap.h
417–419

Was there a typo in the original comment: not listed -> listed?

This revision is now accepted and ready to land.Mar 22 2023, 2:59 AM
ftynse accepted this revision.Mar 22 2023, 3:19 AM
ftynse added inline comments.
mlir/lib/IR/AffineMap.cpp
574–580

Add documentation plz.

721–722

Nit: llvm::is_one_of<> makes this slightly more concise.

724

Nit: camelCase.

731–738

Optional: this is now possible without lambdas, along the lines of

for (...) {
  ...
  if constexpr (isDim) {
    replacements.push_back(getAffineDimExpr(...));
  } else {
    replacements.push_back(getAffineSymbolExpr(...));
  }
}}

which may or may not be more readable.

802

Nit: expand auto (presumably to AffineMap that we don't use by refernece)

nicolasvasilache marked 7 inline comments as done.Mar 22 2023, 5:23 AM
nicolasvasilache added inline comments.
mlir/include/mlir/IR/AffineMap.h
575–576

The issue is that the natural name collides with the compressDims / compressSymbols functions in the .cpp
Do you have a preferred way to spell these names ?

575–576

Please feel free to just commit an NFC on top of this with your preferred spelling.

mlir/lib/IR/AffineMap.cpp
731–738

nice, didn't know about this yet.
not more readable here IMO so not applying but good to know for the future.

802

thanks missed that copypastaing

nicolasvasilache marked 3 inline comments as done.

Address comments.

This revision was landed with ongoing or failed builds.Mar 22 2023, 5:31 AM
This revision was automatically updated to reflect the committed changes.
ftynse added inline comments.Mar 22 2023, 5:33 AM
mlir/lib/IR/AffineMap.cpp
731–738

Yeah, that's what I expected. It is more useful to replace things like enable_if<foo> / enable_if<!foo> pairs.