[PATCH] D156087: [MLIR] Add stage to side effect

[donald chen via Phabricator via llvm-commits]

cxy-1993 added a comment.

In D156087#4552053 <https://reviews.llvm.org/D156087#4552053>, @mehdi_amini wrote:

> I see where you're coming from, but I'm much more on the side of avoiding footgun in our APIs: especially this kind, where it won't be caught until there is a subtle miscompute which requires long hours of debug.

I have already changed the default range of side effects to "partial" and made corresponding modifications to all feasible ops. Thank you for your valuable comments, please review it again.



================
Comment at: mlir/include/mlir/Interfaces/SideEffectInterfaces.td:38
+class MemoryEffect<string effectName, Resource resource, int stage,
+                   bit fullRegion>
+  : SideEffect<MemoryEffectsOpInterface, effectName, resource, stage,
----------------
mehdi_amini wrote:
> cxy-1993 wrote:
> > mehdi_amini wrote:
> > > Do we have enums in TableGen? (I don't remember).
> > > 
> > > I rather read "Full" and "Partial" than true/false if possible.
> > I can not found any enum in TableGen except EnumAttr...
> I think it should look like this in TableGen:
> 
> ```
> class EffectRange <bits<1> val> {
>   bits<1> Value = val;
> }
> 
> def FullEffect : EffectStage<1>;
> def PartialEffect : EffectStage<0>;
> 
> class SideEffect<EffectOpInterfaceBase interface, string effectName,
>                  Resource resourceReference, int effectStage, EffectRange range> {
>    ...
> 
>   // Does this side effect act on every single value of resource.
>   EffectRange effectOnFullRegion = range;
> }
> ```
> 
> But it may be possible to write also here:
> ```
>   // Does this side effect act on every single value of resource.
>   bit effectOnFullRegion = range.Value;
> ```
DONE


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156087/new/

https://reviews.llvm.org/D156087