Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR for llvm/llvm-project#64553 #653

Closed
wants to merge 6 commits into from

Conversation

llvmbot
Copy link
Member

@llvmbot llvmbot commented Aug 28, 2023

…es` to `operandSegmentSizes`

This renaming started with the native ODS support for properties, this is completing it.

A mass automated textual rename seems safe for most codebases.
Drop also the ods prefix to keep the accessors the same as they were before
this change:
 properties.odsOperandSegmentSizes
reverts back to:
 properties.operandSegementSizes

The ODS prefix was creating divergence between all the places and make it harder to
be consistent.

Reviewed By: jpienaar

Differential Revision: https://reviews.llvm.org/D157173
Using properties would break when a dialect isn't in the mlir namespace
…t when initialization is needed

The current logic hashes the context to detect registration changes and re-run
the pass initialization. However it wasn't checking for changes to the
pipeline, so a pass that would get added after a first run would not be
initialized during subsequent runs.

Reviewed By: Mogball

Differential Revision: https://reviews.llvm.org/D158377
…elected fields

It is surprising for the user that only some fields were honored.

Also make the FrozenRewritePatternSet a shared_ptr<const T>.

Fixes #64543

Differential Revision: https://reviews.llvm.org/D157469
@llvmbot
Copy link
Member Author

llvmbot commented Aug 28, 2023

@jpienaar @Mogball @Mogball @jpienaar What do you think about merging this PR to the release branch?

Copy link
Contributor

@Mogball Mogball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Merging these changes in makes sense

@tru
Copy link
Contributor

tru commented Aug 29, 2023

@joker-eph needs a rebase.

@joker-eph
Copy link
Contributor

@tru I updated the branch in my fork, is the bot gonna update this automatically?
(is this documented somewhere?)

@tru
Copy link
Contributor

tru commented Aug 29, 2023

@tru I updated the branch in my fork, is the bot gonna update this automatically? (is this documented somewhere?)

No, you need to comment with the /branch command again.

I don't think it's that well-documented, unfortunately. But it will also have to change for LLVM 18.x since we should be using PRs natively then.

@joker-eph
Copy link
Contributor

Somehow it does not pick it up: llvm/llvm-project#64553 (comment) ?

@tru
Copy link
Contributor

tru commented Aug 30, 2023

Seems to hit some kind of snag in the automation. Want me to manually merge this @joker-eph ?

@joker-eph
Copy link
Contributor

Sure, whatever is easier for you!

@tru
Copy link
Contributor

tru commented Dec 11, 2023

This was merged manually. closing.

@tru tru closed this Dec 11, 2023
@tru tru deleted the joker-eph-mlir-fixes-17.x branch December 11, 2023 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cherry-picks for MLIR in 17.0 release
4 participants