|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.
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.
I can make isSinkingBeneficiary param non-optional and provide isSinkingBeneficiaryDefault explicitly in GpuKernelOutliningPass but I would prefer to do pass splitting in separate review.
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.