[RFC] Add effect index in memroy effect

Background

MLIR has a strong sense of generality, as its ops can represent the computational process of a complete program or an instruction. However, the current description of memory side effects in MLIR only covers the affected values, which is far from sufficient.

Motivation example
If we are trying to implement copy propagation pass on mlir.
Consirder the following input ir:

  ...
  memref.copy %a, %b ...
  xx.compute %b, %b  // Read from %b and write to %b. 
  ...

We cannot infer whether copy propagation can be performed from the intermediate representation above because we don’t know the order of operations in xx.compute - whether the read or write occurs first. If the read happens before the write, then we can eliminate the copy and replace the read operand of xx.compute with %a . However, if the write happens before the read, this substitution would be illegal.

Proposal
We plan to add an index to describe the order of memory side effects. This index will indicate the sequence in which the side effects occur. To ensure backward compatibility with existing code, we will set the default index for all memory side effects to 0. Users will have the option to specify the order of memory side effects either by calling the builder in a specific way or by using a different writing style in the td file.

Changes to the EffectInstance builder:

 EffectInstance(EffectT *effect, Resource *resource = DefaultResource::get(), unsigned index = 0)
     : effect(effect), resource(resource), index(index) {}

Changes to the td files:

def CopyOp : MemRef_Op<"copy", [CopyOpInterface, SameOperandsElementType,           
    SameOperandsShape]> {                                                           
                                                                                    
  let description = [{                                                              
    Copies the data from the source to the destination memref.                      
                                                                                    
    Usage:                                                                          
                                                                                    
    ```mlir                                                                         
    memref.copy %arg0, %arg1 : memref<?xf32> to memref<?xf32>                       
    ```                                                                             
                                                                                    
    Source and destination are expected to have the same element type and shape.
    Otherwise, the result is undefined. They may have different layouts.            
  }];                                                                               
                                                                                    
  let arguments = (ins Arg<AnyRankedOrUnrankedMemRef, "the memref to copy from",
                           [MemReadAt<"0">]>:$source,                               
                       Arg<AnyRankedOrUnrankedMemRef, "the memref to copy to",  
                           [MemWriteAt<"1">]>:$target);                             
                                                                                    
  let assemblyFormat = [{                                                           
    $source `,` $target attr-dict `:` type($source) `to` type($target)              
  }];                                                                               
                                                                                    
  let hasCanonicalizer = 1;                                                         
  let hasFolder = 1;                                                                
}  

Compatibility
All previous builder and td writing styles will remain compatible.

Commit
https://reviews.llvm.org/D156087

Thank you for any suggestion。

A while back, I added parameters to all effect instances to capture additional optional information in a generic way. Have you considered using that?

Thansk for your rely.Parameters to effect instances can solve my problem.

The reason I didn’t use effect parameter is that I believe it provides a capability tailored to different side effects. Side-effect ordering is a common mechanism and will be extensively utilized. For example, some SIMD processor backend dialects have a lot of memory-based computations, such as adding results from two memory segments in parallel and writing the results back to memory. We want to annotate these side-effect orderings heavily in the “td” files.

Another approach is to expose “side effect parameter” to “td” files and agree on the attribute name for side-effect ordering, allowing it to be specified in the “td” files. Which one do you think is more suitable?

It isn’t tailored to a specific kind of effects, that’s why it uses the generic attribute representation. I believe there was a discussion about that, but can seem to find it anymore. Could have been an ODM or something like that though.

That’s a good fit IMO, side effect parameters aren’t extensively utilized in upstream dialects, if at all. Let’s put them to use. If this becomes a performance issue due to memory indirections via attribute storage, it still looks possible to reconsider later on or address the actual problem.

I think it will be better option for the community to expose the parameters in ODS. Thus, if somebody wants to use it for something else than ordering the effects, they will be able to. Note that the attribute name isn’t necessary. The parameter is just an attribute and isn’t stored in a dictionary, unlike the in an op. We should think about the case when there are multiple parameters, which can be solved by either having a dictionary attribute with names as the top-level parameter, or an array attribute with attributes of distinct types, or something else. I am slightly in favor of the second approach with defining a specific SideEffectOrderAttr, which would be also easily visible in ODS.

Thank you for your advice.

It seems that we have reached a consensus not to add parameters with side effects. The storage of side effect order is a attribute named SideEffectOrderAttr under dictionary attribute inside side effect.

I have fixed the patch based on our discussion. Please take some time to review it. By the way, the reason I didn’t directly expose the attributes on the ODS like the properties of ‘op,’ allowing users to write them directly, is that I didn’t want to add a parser to the side-effect interface. Thank you for your guidance.

relate patch:⚙ D156087 [MLIR] Add index to side effect

After discussing with amini, we still believe that it would be more appropriate to use a new field to represent the stages at which side effects occur, rather than directly utilizing parameters.

Therefore, we have opted to employ a new field to represent the stages of side effect occurrences. You can find more detailed information in the following:
https://reviews.llvm.org/D156087#4648015

Discussing privately with someone and just notifying the stakeholders is not how consensus usually works… The discussion should have happened in the RFC to make sure relevant people see it.

1 Like

I think it is more balanced than that: there are some aspects that are fundamental to an RFC and others that are “implementation details”. It’s not unusual that things shifts during the code-review, and it’s not always clear cut to know when things are shifting enough to warrant looping back to the RFC again.
Basically, the ambiguity will always lead to some mishap like here in some cases, it’s good reminder to err on the side of over communicating I guess!

I looked at my comment at the time: seems like effects support a generic attribute for extensibility, the way I see this mechanism is so that when defining a particular effect, the author can add information to them.
We could piggy-back on this generic mechanism here as well, but I don’t think we should because:

  1. We are adding something that is first class and spans across all effects, it’s not specific to a specific effect. I see a bit of a similar distinction to the discardable and inherent attributes on operations.
  2. The attribute is not a dictionary right now (we could make it) so it won’t just compose easily with any usage for specific effect.
  3. Attributes are not efficient: if we’re adding something first-class, I don’t think we should pay the Attribute price (especially when it is just an int!), this is similar to the “Attribute vs Properties” on Operation.
1 Like