This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Analysis] Add batched version of FlatAffineConstraints::addId
ClosedPublic

Authored by springerm on Aug 22 2021, 7:53 PM.

Details

Summary
  • Add batched version of all addId variants, so that multiple IDs can be added at a time.
  • Rename addId and variants to insertId and appendId. Most external users call appendId. Splitting addId into two functions also makes it possible to provide batched version for both. (Otherwise, the overloads are ambigious when calling addId.)

Diff Detail

Event Timeline

springerm created this revision.Aug 22 2021, 7:53 PM
springerm requested review of this revision.Aug 22 2021, 7:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2021, 7:53 PM
ftynse added inline comments.Aug 24 2021, 4:33 AM
mlir/include/mlir/Analysis/AffineStructures.h
248
256

Nit: "at the end of the table" sounds a bit misleading. Do you actually mean insert them after the last identifier of the same kind?

mlir/lib/Analysis/AffineAnalysis.cpp
860

Shouldn't this be appendDimId?

springerm updated this revision to Diff 368535.Aug 24 2021, 7:02 PM
springerm marked 2 inline comments as done.

address comments

springerm added inline comments.Aug 24 2021, 7:03 PM
mlir/lib/Analysis/AffineAnalysis.cpp
860

The original code inserts numCommonLoops many dims at positions 0, 1, 2, ... (i.e., at the beginning). If dependenceDomain does not have any dims yet, this could be rewritten as appendDimId.

Thanks for improving the API. A couple of minor comments.

mlir/include/mlir/Analysis/AffineStructures.h
246–261

-> ... num identifiers ...

670

It is odd to see these using declarations in a header file. I'm trying to understand the purpose.

ftynse added inline comments.Aug 25 2021, 12:13 AM
mlir/include/mlir/Analysis/AffineStructures.h
670

These declarations are not in the header file but in the class. They make overloaded methods of parent class available in the derived class. Otherwise, overload resolution may ignore them when calling a method on the derived class.

springerm marked 2 inline comments as done.Aug 25 2021, 3:33 PM
springerm added inline comments.
mlir/include/mlir/Analysis/AffineStructures.h
670

E.g., it is needed to be able to call appendDimId(/*num=*/5) on FlatAffineValueConstraints. Without the using decl, we get a warning/compile error.

springerm updated this revision to Diff 368759.Aug 25 2021, 3:35 PM
springerm marked an inline comment as done.

rebase

springerm marked an inline comment as done.Aug 25 2021, 3:42 PM
springerm updated this revision to Diff 368769.Aug 25 2021, 4:03 PM

update

mlir/lib/Analysis/AffineStructures.cpp
422

I'm wondering if this is correct.

According to the comment: Local identifiers of each system are by design separate/local and are placed one after other (A's followed by B's).

So I would expect:

a->appendLocalId(/*num=*/b->getNumLocalIds());
ftynse accepted this revision.Aug 27 2021, 12:09 AM
ftynse added inline comments.
mlir/lib/Analysis/AffineStructures.cpp
470

This error looks relevant.

This revision is now accepted and ready to land.Aug 27 2021, 12:09 AM
springerm marked an inline comment as done.Aug 29 2021, 5:42 PM
This revision was landed with ongoing or failed builds.Aug 29 2021, 5:57 PM
This revision was automatically updated to reflect the committed changes.