Page MenuHomePhabricator

Retain attributes of original scf::ForOp when folding
Needs RevisionPublic

Authored by nimiwio on Oct 14 2021, 2:00 PM.

Details

Summary

ForOpIterArgsFolder and ForOpTensorCastFolder construct a new ForOp, but don't retain any pre-existing attributes.

Diff Detail

Event Timeline

nimiwio created this revision.Oct 14 2021, 2:00 PM
nimiwio requested review of this revision.Oct 14 2021, 2:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2021, 2:00 PM

It is a bit tricky, because we can't really know what these attributes are here for. For example they could refer to individual operands of the loop, but we just shuffled them (removed some).

It is a bit tricky, because we can't really know what these attributes are here for. For example they could refer to individual operands of the loop, but we just shuffled them (removed some).

Good point, I had not considered attribute references to operands of the loop.. For context: my situation is that an attribute was added to a ForOp and is being silently dropped during canonicalization (it's triggering ForOpIterArgsFolder).

Would it be possible to try and detect and handle this situation, e.g. keep track of what we modify/remove, and then update/omit any attributes when we copy them over from the original ForOp? I'm not very familiar with all the ways in which attributes may refer to loop operands, so not sure if something like that would be feasible. I'm not really even sure of a way to hack around this for my situation, besides just not running canonicalization, but that seems wrong..

Yes it is messy... Unfortunately our situation with attributes isn't great: we really don't have a good story for this in general :(

bondhugula accepted this revision.Oct 15 2021, 9:25 PM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/lib/Dialect/SCF/SCF.cpp
644

I think we need support on derived op types for a getIntrinsicAttributes() and getExternalAttributes(). In this case, since the op's iter args, results, and iter operands are being trimmed and the op doesn't have any intrinsic attributes that correspond to the cross-iter value flow/init and yield, the compiler developer of this pattern knows that it's correct to propagate all external attributes as is. An inherent assumption is that the transforms and rewrites on the ops have the guarantee that any user-defined and external attributes by design don't have an impact on the core semantics of the op and it should be correct for them to propagate them as is. Had those attributes been core attributes, they'd have to be appropriately handled. A corollary of this is that those attaching user-defined attributes shouldn't expect any special handling and treatment on propagation of those.

So what we need is a getExternalAttributes() and just propagate those. In this case, scf.for has zero intrinsic attributes and so this should be fine. But please a comment/warning on a future update once proper support is available.

This revision is now accepted and ready to land.Oct 15 2021, 9:25 PM

This revision itself looks good to me modulo the code comment to be added.

mehdi_amini requested changes to this revision.Oct 16 2021, 2:42 PM
mehdi_amini added inline comments.
mlir/lib/Dialect/SCF/SCF.cpp
644

I don't think I'm in agreement on the safety here: the safe things generic transformations should do is discard unknown attributes.

This revision now requires changes to proceed.Oct 16 2021, 2:42 PM
bondhugula added inline comments.Oct 16 2021, 6:48 PM
mlir/lib/Dialect/SCF/SCF.cpp
644

And I would invert it and argue the other way: the *safer* or equally safe thing to do is to *preserve* external attributes. Besides safety or if all else is the same on safety, propagating prevents loss of information and provides the desired functionality to pass pipeline developers that are using these external attributes to propagate short-lived information.

I hope you mean the same thing by "unknown" as I mean by "external" -- these are attributes that are not part of an op's definition and one can't expect any loss is safety in propagating those. If one argues that propagating them is unsafe, one could argue that dropping them is more unsafe.

mehdi_amini added inline comments.Oct 16 2021, 7:15 PM
mlir/lib/Dialect/SCF/SCF.cpp
644

I don't think I adhere to your "inversion" right now, but let's keep it for Discourse.

Thank you both for the discussion.. I'm not quite sure how to proceed on this though. As things stand right now, it seems like it is not safe for a pass to add custom attributes to a ForOp. Is this true for any Ops belonging to a Dialect not "owned" by a pass? The notion of "external" vs "intrinsic" attributes seems to make sense to me but if there's not agreement on that, is there some other way that we can propagate custom information about an Op besides through attributes? I'm not necessarily attached to this change, but I'm just not sure of a workable alternative right now.