Page MenuHomePhabricator

[mlir][gpu] sinkOperationsIntoLaunchOp: Add user hook for isSinkingBeneficiary

Authored by Hardcode84 on Feb 12 2022, 8:20 AM.

Diff Detail

Event Timeline

Hardcode84 created this revision.Feb 12 2022, 8:20 AM
Hardcode84 requested review of this revision.Feb 12 2022, 8:20 AM
mehdi_amini added inline comments.Feb 12 2022, 11:01 AM
30 ↗(On Diff #408183)

I'm concerned about making any pass non hermetic: that mean adding any C++ injection that won't be serialized in the textual pipeline.

I'd like to keep the pass simple and hermetic, and extract whatever logic as utilities so that one can customize in their own pass instead.

revert pass changes, add unittest

fix licence header

Thank you. The default is fairly arbitrary (well, it's good enough for some index computations) and we really should have some cost model here. +1 to making it configurable at least.


It would be nice to maybe split the pass into two, one that does sinking and one that does outlining. Then we could remove the default here completely and add it to the sinking pass instead. Actual users of this API would need to implement their own cost model.

Hardcode84 added inline comments.Feb 14 2022, 9:46 AM

I can make isSinkingBeneficiary param non-optional and provide isSinkingBeneficiaryDefault explicitly in GpuKernelOutliningPass but I would prefer to do pass splitting in separate review.

make isSinkingBeneficiary param non-optional

mehdi_amini added inline comments.Feb 14 2022, 11:55 AM
57 ↗(On Diff #408490)

Please keep the testing implemented with passes, this is how MLIR is tested in general.

Hardcode84 added inline comments.Feb 14 2022, 1:16 PM
57 ↗(On Diff #408490)

I that case I can just drop these test and assume it is tested by existing GpuKernelOutliningPass tests.

herhut accepted this revision.Feb 15 2022, 3:04 AM

Thank you. Just a naming nit.


Sounds good. Maybe also remove the 'Default' from the name then. We could even call it isLikelyAnIndexComputation or something because that is what the function really does.

57 ↗(On Diff #408490)

If we do not have a default implementation, then this test is not needed indeed. The other test would already test that providing an implementation works and we do not have to test the overriding.

This revision is now accepted and ready to land.Feb 15 2022, 3:04 AM

rebase, remove unittest, rename default func