This is an archive of the discontinued LLVM Phabricator instance.

Add capture of "IRUnits" as context for an MLIR Action
ClosedPublic

Authored by mehdi_amini on Feb 25 2023, 10:19 PM.

Details

Summary

IRUnit is defined as:

using IRUnit = PointerUnion<Operation *, Region *, Block *, Value>;

The tracing::Action is extended to take an ArrayRef<IRUnit> as context to
describe an Action. It is demonstrated in the "ActionLogging" observer.

Diff Detail

Event Timeline

mehdi_amini created this revision.Feb 25 2023, 10:19 PM
mehdi_amini requested review of this revision.Feb 25 2023, 10:19 PM
Tyker added a subscriber: Tyker.Mar 1 2023, 6:22 PM

The data-flow analysis framework has a concept very similar to IRUnit its called ProgramPoint maybe this should be shared ?

Mogball accepted this revision.Mar 6 2023, 11:45 AM
Mogball added a subscriber: Mogball.
Mogball added inline comments.
mlir/include/mlir/IR/Unit.h
18
25
32
43

newline

mlir/lib/IR/AsmPrinter.cpp
681 ↗(On Diff #500571)

where does the 2 come from?

This revision is now accepted and ready to land.Mar 6 2023, 11:45 AM
rriddle added inline comments.Mar 8 2023, 10:17 PM
mlir/include/mlir/IR/Unit.h
26

It'd be nice not to document this in the context of actions, I'd rather have something be general.

mlir/lib/Debug/Observers/ActionLogging.cpp
10–13

Some of these could be dropped? Like Region.h?

47–48

Can you add a stream operator and simply this?

mlir/lib/IR/AsmPrinter.cpp
228 ↗(On Diff #500571)

It'd be nice to split this change out, it feels separable. I think we could also use this here: https://github.com/llvm/llvm-project/blob/73058e330134d275070d49dba80e5fda50ec015b/mlir/lib/Tools/mlir-lsp-server/MLIRServer.cpp#L519

The data-flow analysis framework has a concept very similar to IRUnit its called ProgramPoint maybe this should be shared ?

I'm not sure how to unify it, it feels that somehow the ProgramPoint could inherit from IRUnit but it is declared as a more extensible framework: public PointerUnion<GenericProgramPoint *, Operation *, Value, Block *> so I'm not sure how to unify.

mehdi_amini marked 8 inline comments as done.

address comments

mlir/include/mlir/IR/Unit.h
26

Yeah that's a remainder from a time where this was defined inside the Action.h header.

mlir/lib/IR/AsmPrinter.cpp
228 ↗(On Diff #500571)

What about updating MLIRServer to use this new flag?

rriddle accepted this revision.Mar 13 2023, 12:02 AM
rriddle added inline comments.
mlir/include/mlir/IR/OperationSupport.h
867 ↗(On Diff #504478)

Missing a rebase?

mlir/include/mlir/IR/Unit.h
35

Should be able to drop most of the llvm:: in this file.

mlir/lib/IR/Unit.cpp
42–46

Can you just do std::distance instead (using block->getIterator())?

55–58

Feels cleaner, but feel free to ignore

mehdi_amini marked an inline comment as done.Mar 13 2023, 8:07 AM
mehdi_amini added inline comments.
mlir/include/mlir/IR/OperationSupport.h
867 ↗(On Diff #504478)

Yeah haven't updated this patch since I wrote the other one :)

mehdi_amini marked an inline comment as done.

Rebase & address comments

mehdi_amini marked an inline comment as done.

One more comment addressed

mehdi_amini marked an inline comment as done.Mar 13 2023, 8:54 AM

The data-flow analysis framework has a concept very similar to IRUnit its called ProgramPoint maybe this should be shared ?

I'm not sure how to unify it, it feels that somehow the ProgramPoint could inherit from IRUnit but it is declared as a more extensible framework: public PointerUnion<GenericProgramPoint *, Operation *, Value, Block *> so I'm not sure how to unify.

If anyone gets an idea, we'll refactor then, in the meantime I'll push this as-is!

This revision was landed with ongoing or failed builds.Mar 20 2023, 5:47 AM
This revision was automatically updated to reflect the committed changes.