In D94638#2503020, @bollu wrote:

Out of curiosity: may I please have some context for what this base class is being created for? Ie, what consumers other than the existing PatternRewriter will soon exist? :)

Address most comments

Address comments

Rebase

Fix accidental incorrect behavior for casts

Did a pass through, and looks good to me.

In D94662#2499581, @youngar wrote:

Should I get more reviews, or should I arc land this now?

Thanks!

Awesome!

I went with IRRewriterImpl as this seems close to a SmallVectorImpl situation, but happy to rename to something else if there are suggestions.

Resolve comments and rebase

+Chris as I think you also ran into a need for this.

LGTM from me, but can you send a post to llvm-dev to make sure there is general consensus?

In D94191#2483685, @csigg wrote:

Separate definition because gcc 10 doesn't allow attributes on function definition.

It's done now, but it seems to me that the C++14 attribute would have been cleaner.

LLVM uses C++14, so I'm wondering if the LLVM_ATTRIBUTE_DEPRECATED macro itself should be deprecated ;-)

Nice!

LGTM after adding error messages. Thanks!

In D94200#2483234, @rriddle wrote:

In D94200#2483226, @jurahul wrote:

This is in preparation of the visibility related changes discussed in https://llvm.discourse.group/t/external-function-declaration-has-changed-help-needed/2357. This change disallows attributes that sym_visibility/sym_name/type etc which are parsed/inferred from other places to be present in the attribute dictionary. This way, for the follow on changes, the function parser has just one parsed visibility value (parsed as keyword private/public/nested) to consider instead of trying to also look into the parsed attribute dictionary.

I don't see why you can't just check these in FunctionImplementation.cpp. Why do you need to add more flags to the Parser methods?

In D94200#2483226, @jurahul wrote:

This is in preparation of the visibility related changes discussed in https://llvm.discourse.group/t/external-function-declaration-has-changed-help-needed/2357. This change disallows attributes that sym_visibility/sym_name/type etc which are parsed/inferred from other places to be present in the attribute dictionary. This way, for the follow on changes, the function parser has just one parsed visibility value (parsed as keyword private/public/nested) to consider instead of trying to also look into the parsed attribute dictionary.

Rebase

Rebase

Rebase

Rebase

Nice!

Thanks!

In D93147#2449933, @rriddle wrote:

In D93147#2449930, @rriddle wrote:

In D93147#2449929, @mehdi_amini wrote:

There is something subtle here: re-invoking the pass manager twice after having modified the context (like loading more dialect) may not reinitialize the passes correctly.
For example this fictional API:

std::pair<ModuleOp, ModuleOp> canonicalize_two_files(PassManager &PM, MLIRContext *ctx, StringRef file1, StringRef file2) {
ModuleOp m1 = parseMlirFile(ctx, file1);
PM.run(m1);
ModuleOp m2 = parseMlirFile(ctx, file2);
PM.run(m2);
return {m1, m2};
}

Yes. The main reason to cache the initialization is to properly handle the dynamic pipeline case. For the static pipeline, we can guarantee that it is only done once per full execution. For dynamic pipelines, we have no such guarantee or knowledge that it was initialized. We could enforce that the user initializes the pipeline.

I don't particularly like the caching, but pushed to phab so we can discuss it here.

Change to using "Generations" for initialization, and add docs

How does this break the Tensorflow test suite? Can you add more context?