This is an archive of the discontinued LLVM Phabricator instance.

[mlir][linalg] Update Structured Op Interface (NFC).
ClosedPublic

Authored by gysit on May 31 2021, 3:53 AM.

Details

Summary

Adding methods to access operand properties via OpOperands and mark outdated methods as deprecated.

Diff Detail

Event Timeline

gysit created this revision.May 31 2021, 3:53 AM
gysit requested review of this revision.May 31 2021, 3:53 AM
nicolasvasilache accepted this revision.May 31 2021, 5:34 AM

LGTM, thanks for modernizing this!

This revision is now accepted and ready to land.May 31 2021, 5:34 AM
This revision was landed with ongoing or failed builds.May 31 2021, 6:23 AM
This revision was automatically updated to reflect the committed changes.

This change is causing the following warning when building with clang:

/mnt/vss/_work/1/s/mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp:332:24: error: comparison of integers of different signs: 'std::size_t' (aka 'unsigned long') and 'int64_t' (aka 'long') [-Werror,-Wsign-compare]
      if (view.index() >= op.getNumInputs())
          ~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~

Changing the return type of a function is not generally considered a NFC.

Changing the return type of a function is not generally considered a NFC.

It is from the point of view of the compiler: it won't affect its behavior in any way.
A good litmus test is: "do we need a test for this patch?" ; if not then it is likely NFC.

(fixed the warning in 8c948b18e9d8 by the way)

Changing the return type of a function is not generally considered a NFC.

It is from the point of view of the compiler: it won't affect its behavior in any way.
A good litmus test is: "do we need a test for this patch?" ; if not then it is likely NFC.

That is a good test, but I don't think it's sufficient. In this particular case, the change causes warnings to be produced by the compiler that previously weren't, so even if no new tests were needed, it is still not a NFC. If the return type is relied upon elsewhere in code, I would expect that is a functional change. If the code never tests the return type, then changing it would be NFC.

In general, I think using NFC less often than strictly possible is better than using it more often, because as someone who looks at relevant commits when I see a failure in my build/tests/etc., I would hope that I can ignore the NFC changes and just focus on changes that are considered functional.

(fixed the warning in 8c948b18e9d8 by the way)

Thanks!

gysit added a comment.Jun 2 2021, 11:50 AM

(fixed the warning in 8c948b18e9d8 by the way)

Thanks for fixing this!