This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] FlatAffineValueConstraints: Fix bug in mergeSymbolIds
ClosedPublic

Authored by Groverkss on Oct 17 2021, 1:57 AM.

Details

Summary

This patch fixes a bug in implementation mergeSymbolIds where symbol
identifiers were not unique after merging them. Asserts for checking uniqueness
before and after the merge are also added. The asserts checking uniqueness
after the merge fail without the fix on existing test cases.

Diff Detail

Event Timeline

Groverkss created this revision.Oct 17 2021, 1:57 AM
Groverkss requested review of this revision.Oct 17 2021, 1:57 AM

Is there a way to add a test case for this?

Is there a way to add a test case for this?

The existing test cases fail on adding the assert. Do you prefer a test case that fails without the asserts too? I do not have a test case for that as of now, but I can try to find it.

Groverkss edited the summary of this revision. (Show Details)Oct 17 2021, 2:08 AM

The update of the commit message is enough, from my side.

arjunp added inline comments.Oct 18 2021, 3:22 PM
mlir/lib/Analysis/AffineStructures.cpp
451–454

This may stop working if the variable layout changes. I would prefer this implementation to not depend on that.

You could change this to take e.g. start and end indices or offset and length parameters. You could also write an additional function e.g. areSymbolIdsUnique that calls into this for your use case.

Groverkss updated this revision to Diff 380856.Oct 19 2021, 9:50 PM
Groverkss marked an inline comment as done.
  • Address Arjun's comments

Addressed @arjunp's comments

arjunp added inline comments.Oct 19 2021, 10:31 PM
mlir/lib/Analysis/AffineStructures.cpp
465

Here and below: I think the end of the sentence is missing. Check what?

472

Same here.

Groverkss marked 2 inline comments as done.
  • Fix doc comments

Fixed doc comments.

arjunp accepted this revision.Oct 24 2021, 6:57 AM

Seems good to me. Minor comments below.

mlir/lib/Analysis/AffineStructures.cpp
457

Maybe assert that end < maybeValues.size()

609

Maybe not for this patch but it would be nice to also make this not depend on the variable layout, maybe calling into some function to get an IdKind from a location.

This revision is now accepted and ready to land.Oct 24 2021, 6:57 AM
Groverkss updated this revision to Diff 381783.Oct 24 2021, 7:19 AM
Groverkss marked an inline comment as done.
  • Add asserts for areIdsUnique
  • Use range based for loop in areIdsUnique

Addressed comments.