This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Analysis] Add FlatAffineConstraints::addLowerOrUpperBound
ClosedPublic

Authored by springerm on Aug 8 2021, 11:02 PM.

Details

Summary

This function overload is similar to the existing FlatAffineConstraints::addLowerOrUpperBound. It constrains a dimension based on an affine map. However, in contrast to the other overloading, it does not attempt to align dimensions/symbols of the affine map with the dimensions/symbols of the constraint set. Instead, dimensions/symbols are expected to already be aligned.

Depends On D107726

Diff Detail

Event Timeline

springerm created this revision.Aug 8 2021, 11:02 PM
springerm requested review of this revision.Aug 8 2021, 11:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2021, 11:02 PM
ftynse accepted this revision.Aug 9 2021, 8:17 AM
ftynse added inline comments.
mlir/lib/Analysis/AffineStructures.cpp
2055

Let's omit the number of stack elements in SmallVector unless there is a good reason to choose a specific number.

2060

If it's invalid, should this be an assertion instead of continue?

This revision is now accepted and ready to land.Aug 9 2021, 8:17 AM
springerm marked an inline comment as done.Aug 9 2021, 10:41 PM
springerm added inline comments.
mlir/lib/Analysis/AffineStructures.cpp
2060

I think so. But some existing code/test cases would fail. E.g., FlatAffineConstraints::addDomainFromSliceMaps adds invalid bounds here and relies on the fact that the other (non-invalid) inequalities are being added:

// This slice refers to a loop that doesn't exist in the IR yet. Add its
// bounds to the system assuming its dimension identifier position is the
// same as the position of the loop in the loop nest.
if (lbMap && failed(addLowerOrUpperBound(i, lbMap, operands, /*eq=*/false,
                                         /*lower=*/true)))
  return failure();

Looks like a bug in addDomainFromSliceMaps or one of its callers. Maybe better to fix it now. Taking a look...

springerm added inline comments.Aug 10 2021, 1:38 AM
mlir/lib/Analysis/AffineStructures.cpp
2060

I don't know enough about the code base (in particular ComputationSliceState) to fix this. Putting a TODO in here for now, and continuing with the original refactoring.

This revision was landed with ongoing or failed builds.Aug 10 2021, 11:14 PM
This revision was automatically updated to reflect the committed changes.

Great to see this refactoring chain, some fly-by comments.

mlir/lib/Analysis/AffineStructures.cpp
2025

I find this somewhat unfortunate.
How about addBound(unsigned pos, AffineMap boundMap, BoundType type) where enum class BoundType { EQ, LB, UB }; ?

2045

While we are refactoring and cleaning up, I would drop all these one off functions and have a single, batched:
addIds(IdType idType, int pos, int numIds = 1);
where enum class IdType { Dim, Sym, Local };

The all this could be simply rewritten as:

if (...) {
  addIds(IdType::Local, 0,  localCst.getNumLocalIds());
  localCst.addIds(IdType::Local, localCst.getNumLocalIds(), getNumLocalIds());
  append(localCst);
}
springerm marked an inline comment as done.Aug 21 2021, 4:56 PM
springerm added inline comments.
mlir/lib/Analysis/AffineStructures.cpp
2025

Addressed in a subsequent commit.

2045

It's on my TODO list.