This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Add support for extracting an integer sample point (if one exists) from an unbounded FlatAffineConstraints.
ClosedPublic

Authored by arjunp on Jan 20 2021, 7:06 AM.

Details

Summary

With this, we have complete support for finding integer sample points in FlatAffineConstraints.

Diff Detail

Event Timeline

arjunp created this revision.Jan 20 2021, 7:06 AM
arjunp requested review of this revision.Jan 20 2021, 7:06 AM
bollu added inline comments.Jan 20 2021, 1:16 PM
mlir/lib/Analysis/AffineStructures.cpp
1193–1194

Consider moving the line

cpp
  FlatAffineConstraints boundedSet = transformedSet;

below the line

// 2) Remove the unbounded dimensions and constraints involving them to
// obtain a bounded set.

?

That way, it's a little less jarring to see boundedSet = transformedSet as the comment explains what's going on.

1198–1259

consider

const unsigned numBoundedDims = result.first;
const unsigned numUnboundedDims = getNumIds() - numBoundedDims;

to aid the reader that these are not modified.

1312

consider:

SmallVector<int64_t, 8> coneSample(llvm::map_range(shrunkenConeSample, ceil));

I believe the llvm::map_range lives in

llvm/ADT/STLExtras.h
2286

Consider adding a comment about how setting a variable is equivalent to updating the constant term and then removing the Id?

mlir/lib/Analysis/LinearTransform.cpp
113–114

I believe this should be

result.reserve(matrix.getNumColumns());

consider:

SmallVector<int64_t, 8> result(0, matrix.getNumColumns());
  for (unsigned col = 0, e = matrix.getNumColumns(); col < e; ++col)
    for (unsigned i = 0, e = matrix.getNumRows(); i < e; ++i)
      result[col] += rowVec[i] * matrix(i, col);
132

I believe the reserve should be:

result.reserve(rowVec.size());

Consider rewriting the body as:

// show reserve rowVec.size() ?
SmallVector<int64_t, 8> result(0, rowVec.size());
// eliminate intermediate `elem`
for (unsigned row = 0, e = matrix.getNumRows(); row < e; row++)
  for (unsigned i = 0, e = matrix.getNumColumns(); i < e; i++)
    result[row] += matrix(row, i) * colVec[i];
arjunp updated this revision to Diff 318316.Jan 21 2021, 2:23 PM
arjunp marked 4 inline comments as done.

Addressed most of Siddharth's comments.

arjunp updated this revision to Diff 318317.Jan 21 2021, 2:28 PM

Address another comment

arjunp marked an inline comment as done.Jan 21 2021, 2:29 PM
ftynse accepted this revision.Jan 22 2021, 4:59 AM
ftynse added inline comments.
mlir/lib/Analysis/AffineStructures.cpp
1149–1150

I don't understand what it means for a vector v to be a solution to a set. Do you mean the vector belongs to the set?

1198–1259

Like I said in another review, MLIR does not follow this pattern, and many core contributors request the inverse change in their reviews. It is not a codified rule, but we have a strong preference for using a consistent style across the code base. If you prefer we used this pattern in MLIR, please make your case for it and post it on https://llvm.discourse.group/c/mlir/31, folks will be happy to discuss it and reach consensus. Until then, it might be preferable to keep the status quo, i.e., not using this pattern.

1227–1231

Nit: took me some time to convince myself this was okay. In particular, why is this considering e_i = 1 after saying 0 <= e_i < 1 in the previous line (handling the case of e_i = 1 automatically handles the entirety of possible e_i). An example could help IMO

1262

Nit: sample.append. I would also move-construct sample from boundedSample to spare a copy.

2280–2295

ArrayRef<T> is implicitly constructible from T, no need for this magic incantation here. I suppose you can even drop the setAndEliminate(unsigned, int64_t) overload entirely and everything will keep working.

This revision is now accepted and ready to land.Jan 22 2021, 4:59 AM
bollu added inline comments.Jan 22 2021, 7:21 AM
mlir/lib/Analysis/AffineStructures.cpp
1198–1259

(Unfortunately, the link above does not work @ftynse; I'd be happy to read a case for disallowing local constants).

I know the larger case that is made here: https://mlir.llvm.org/docs/Rationale/UsageOfConst/

but that argues about const members of classes; not about local primitive const values. I don't see the problem with const-ing unsigned variables *in local scope* ?

arjunp updated this revision to Diff 318524.Jan 22 2021, 7:35 AM
arjunp marked 4 inline comments as done.

Addressed Alex's comments.

mlir/lib/Analysis/AffineStructures.cpp
1149–1150

Yes, changed it to say that.

1227–1231

Added an example and a more detailed explanation.

1262

Just made sample a reference to boundedSample.

2280–2295

Dropped the overload.

ftynse added inline comments.Jan 22 2021, 9:06 AM
mlir/lib/Analysis/AffineStructures.cpp
1198–1259

Strange, the link does work for me. Anyway, it's the usual MLIR discussion forum, there are links to it from mlir.llvm.org.

I generally dislike unnecessary syntactic verbosity, but that's beyond the point here. MLIR code base may or may not follow some patterns that are not codified in the style description. We don't use const on local variables if it is not part of the type (i.e., const reference), we don't use explicit inline on free functions in source files, and we attach * and & to the name rather than the type, to name a few. We use the range abstraction when possible and prefix getters with get. We prefer getters to public const fields. All these are implicit, and that's one of the reasons why we do code review. There is a general rule of following the style of the surrounding code, so if there somehow is a file in the code base that has different patterns, those may prevail. Now, for these relatively new files, the surrounding code is the rest of the code base. Introducing the patterns that locally diverge from the rest of the code base makes life harder for anybody who has to work a lot across the code base, usually the people who will review and maintain this code, and perform large-scale changes later. So I care a lot about consistency of this code with the rest of the code base. I am open to discussing the patterns we prefer to use or not as long as this discussion applies to the entire code base and not a single file, directory, subsystem or patch author. Since you are the one who suggests to diverge from the established practice, it sounds reasonable that the burden of starting the discussion and reaching consensus is on you.

bondhugula added inline comments.Jan 23 2021, 3:52 AM
mlir/lib/Analysis/AffineStructures.cpp
1198–1259

The * and & attachment to the name comes directly from LLVM's style which we aren't overriding. clang-tidy/format is already set to catch and fix these. The MLIR coding style only says where we diverge from the LLVM style. For the rest that don't find a mention in the LLVM as well as MLIR style guides, it's normally expected that a developer will use what they think is better. We should include more in the style file if a specific pattern is being consistently used and is preferred for consistency -- or else, it just leads to more review-time burden for both reviewers and contributors.