This is an archive of the discontinued LLVM Phabricator instance.

Add tracing for pattern application in a ApplyPatternAction
ClosedPublic

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

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

LGTM but River mentioned there were some perf issues. Do have a quick capture of the before vs. after time to run a pattern-heavy workload?

LGTM but River mentioned there were some perf issues. Do have a quick capture of the before vs. after time to run a pattern-heavy workload?

I didn't benchmark but dumped the assembly and exchanged with River, he was fine with the current state. We'll pay the price of the capture only (here setting up two pointers on the stack).

Update with one extra no-inline indirection

rriddle accepted this revision.Mar 13 2023, 12:15 AM
rriddle added inline comments.
mlir/lib/Rewrite/PatternApplicator.cpp
188–192

Seeing this pattern multiple times almost makes me think executeAction should have a return value, but that's a weak thought.

This revision is now accepted and ready to land.Mar 13 2023, 12:15 AM
mehdi_amini added inline comments.Mar 14 2023, 8:55 AM
mlir/lib/Rewrite/PatternApplicator.cpp
188–192

Do you have some C++ dark-magic ideas about this? It's not clear to me to see how to build some return value with unknown type ahead of time there (unless you mean we'd just say we'll forward a bool and that's it.

This revision was landed with ongoing or failed builds.Apr 10 2023, 5:43 PM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Apr 10 2023, 6:18 PM
mlir/lib/Rewrite/PatternApplicator.cpp
188–192

Still interested in this if you have thoughts!