Page MenuHomePhabricator

Use an Identifier instead of an OperationName internally for OpPassManager identification (NFC)
ClosedPublic

Authored by mehdi_amini on Sep 1 2020, 12:56 AM.

Details

Summary

This allows to defers the check for traits to the execution instead of forcing it on the pipeline creation.
In particular, this is making our pipeline creation tolerant to dialects not being loaded in the context yet.

Diff Detail

Event Timeline

mehdi_amini created this revision.Sep 1 2020, 12:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2020, 12:56 AM
mehdi_amini requested review of this revision.Sep 1 2020, 12:56 AM
mehdi_amini added inline comments.Sep 1 2020, 12:58 AM
mlir/lib/Pass/Pass.cpp
342

Check is delayed here now FYI

mehdi_amini edited the summary of this revision. (Show Details)Sep 1 2020, 12:58 AM
GMNGeoffrey accepted this revision.Sep 1 2020, 9:48 AM

Thanks!

mlir/lib/Pass/Pass.cpp
342

I think the distinction in the error message between "not registered" and "not IsolatedFromAbove" was useful and should be preserved. I predict someone will otherwise spend a while trying to figure out why it doesn't like their operation that is marked "IsolatedFromAbove"

502–503

While you're here, fix this typo?

This revision is now accepted and ready to land.Sep 1 2020, 9:48 AM
rriddle added inline comments.Sep 1 2020, 9:54 AM
mlir/include/mlir/Pass/PassInstrumentation.h
17–18

These should be sorted.

46

These can be changed to value-typed now.

mlir/include/mlir/Pass/PassManager.h
31–32

These should be sorted.

73

This should be value-typed.

96

Same here.

mlir/lib/Pass/Pass.cpp
338

Return the error directly, it converts to failure

502–503

It wasn't a typo, iff means if and only if. It's a logical conjunction.

mlir/unittests/Pass/PassManagerTest.cpp
79

nit: Remove the double public and just make this a struct.

86

The explicit StringRef seems unnecessary here.

88

Virtual here is unnecessary.

GMNGeoffrey added inline comments.Sep 1 2020, 9:56 AM
mlir/lib/Pass/Pass.cpp
502–503

That wasn't the typo I was referring to. I meant "the any" :-D

rriddle added inline comments.Sep 1 2020, 9:57 AM
mlir/lib/Pass/Pass.cpp
502–503

Nice!

mehdi_amini marked 14 inline comments as done.Sep 1 2020, 1:21 PM
mehdi_amini added inline comments.
mlir/lib/Pass/Pass.cpp
342

Sure, I just added it back.
Note that it is hard to get a non-registered op at this point (you'd need to programmatically build the pipeline *and* pass --allow-unregistered-dialects I think)
The previous error was needed because it would have caught misconfigurations.

This is some check that we're losing by the way: with the dialects registered "late" we would have to do more work to catch this.

502–503

Since the upgrade comments can be made on a specific subset of a line, if you hover over @GMNGeoffrey 's comment you see the highlighted section :)

mlir/unittests/Pass/PassManagerTest.cpp
86

no known conversion from 'const char [11]' to 'Optional<llvm::StringRef>

Any alternative?

rriddle added inline comments.Sep 1 2020, 1:22 PM
mlir/unittests/Pass/PassManagerTest.cpp
86

Ah right, forgot about the Optional. Keeping it is fine.

mehdi_amini marked 3 inline comments as done.

Address comments

Remove forward declaration after adding header include

Harbormaster completed remote builds in B70293: Diff 289270.
rriddle added inline comments.Sep 1 2020, 2:59 PM
mlir/include/mlir/Pass/PassInstrumentation.h
12

Why do you need the include in this file?

46

nit: Drop the const on all of these.

rriddle accepted this revision.Sep 1 2020, 3:12 PM
rriddle added inline comments.
mlir/unittests/Pass/PassManagerTest.cpp
81

Is this actually necessary to provide for this test?

119

Why the change between expect and assert here?

mehdi_amini marked 2 inline comments as done.Sep 1 2020, 3:30 PM
mehdi_amini added inline comments.
mlir/include/mlir/Pass/PassInstrumentation.h
12
ERROR: mlir/include/mlir/Pass/PassInstrumentation.h:46:51 variable has incomplete type 'const mlir::Identifier'
  virtual void runBeforePipeline(const Identifier name,
mlir/unittests/Pass/PassManagerTest.cpp
81

It's not, I'll remove.

119

If I don't assert, the test continues and it'll crash on the next line.

mehdi_amini marked 2 inline comments as done.

Rebase on D86994, reduce the amount of Identifier::get() calls.