This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Switch arith, llvm, std & shape dialects to prefixed both form.
ClosedPublic

Authored by jpienaar on Oct 24 2021, 10:18 AM.

Details

Summary

Following
https://llvm.discourse.group/t/psa-ods-generated-accessors-will-change-to-have-a-get-prefix-update-you-apis/4476,
this follows flipping these dialects to _Both prefixed form. This
changes the accessors to have a prefix. This was possibly mostly without
breaking breaking changes if the existing convenience methods were used.
The following changes were made to overlapping names & warnings for
skipped:

LLVM::InvokeOp: operands -> callee_operands
LLVM::Global: type -> global_type
GenericAtomicRMW: body -> unnamed (SingleBlock so getBody is defined for unnamed regions)

(https://github.com/jpienaar/llvm-project/blob/main/clang-tools-extra/clang-tidy/misc/AddGetterCheck.cpp
was used to migrate the callers post flipping, using the output from Operator.cpp)

Diff Detail

Event Timeline

jpienaar created this revision.Oct 24 2021, 10:18 AM
jpienaar requested review of this revision.Oct 24 2021, 10:18 AM

This will be a breaking change for many people, can you expand in the description (even the title is cryptic for anyone not familiar with the context of the migration).
I don't know if you have a script or something to help downstream to update, but it is worth giving advice in the description as well.

Otherwise LG :)

This will be a breaking change for many people, can you expand in the description (even the title is cryptic for anyone not familiar with the context of the migration).
I don't know if you have a script or something to help downstream to update, but it is worth giving advice in the description as well.

Otherwise LG :)

With prefix both, there ended up being 0 breaking change (well unless one accesses the attributes raw and then there are 3), good idea to duplicate the script used in description.

jpienaar edited the summary of this revision. (Show Details)Oct 24 2021, 5:21 PM
jpienaar updated this revision to Diff 381812.Oct 24 2021, 5:22 PM
jpienaar edited the summary of this revision. (Show Details)

Add more functions to reduce number of breaking changes.

With prefix both, there ended up being 0 breaking change

Ah! Good point, I missed the fact that you used the "both" mode.

(well unless one accesses the attributes raw and then there are 3)

I think the change in attribute names will lead to C++ accessor changed, every in "both" mode, but yeah that's minor.

(well unless one accesses the attributes raw and then there are 3)

I think the change in attribute names will lead to C++ accessor changed, every in "both" mode, but yeah that's minor.

Actually can you push this in multiple commits? It is always much easier to have this in isolation (renaming the 3 attributes looks like something that can be standalone ahead of the rest)

(well unless one accesses the attributes raw and then there are 3)

I think the change in attribute names will lead to C++ accessor changed, every in "both" mode, but yeah that's minor.

Correct, except I added forwarding functions (well in one case it already existed, I just changed the internal function :)).

Actually can you push this in multiple commits? It is always much easier to have this in isolation (renaming the 3 attributes looks like something that can be standalone ahead of the rest)

Good idea

jpienaar edited the summary of this revision. (Show Details)Oct 24 2021, 5:57 PM
jpienaar edited the summary of this revision. (Show Details)
jpienaar edited the summary of this revision. (Show Details)
This revision was not accepted when it landed; it landed in state Needs Review.Oct 24 2021, 6:36 PM
This revision was automatically updated to reflect the committed changes.