In D156087#4652100, @mehdi_amini wrote:

ftynse acknowledged my rationale with a "like" on Discourse, so I suspect this is good to go?

rebase main

This patch has already been rebased onto the main branch and passed precheckin. I don't have write access to the LLVM repo. Could you please help me to push this code into the master, Thanks. @Mogball

rebase main

In D156087#4647473, @ftynse wrote:

DIdn't the forum RFC conclude on using the parameters attribute for this instead of adding new fields?

@rriddle ping :) Could you please spare some time to review this patch, thank you!

@rriddle Please help me review this piece of code. Thank you for your time. :)

@rriddle Ping :)

@rriddle @mehdi_amini Could you kindly spare a moment to review the updated code at your convenience? Thank you for your time and consideration.

ping @mehdi_amini @rriddle Could you please take some time to help me with the review and merging of this patch? Thank you. :)

Ping @rriddle Could you please help me review this piece of code? Thank you! :)

Update docs

In D156087#4555106, @Mogball wrote:

+1. This patch makes sense to me. Having this level of granularity is useful.

Do any of the upstream transformations need to be taught about this?

I have already modified the default range of side effects to "partial" and made necessary adjustments to various ops based on your feedback. I am reaching out to kindly request another code review for the changes I have made. @mehdi_amini

In 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.

Update patch according to review comments

In D156087#4551469, @mehdi_amini wrote:

I believe that when we say “write full” to a particular resource, this write is certain to occur. Therefore, any writes to this resource before that point can be killed by this operation. If the write is only possible, and we shouldn't explicitly mark it as writing all resources, do you think this approach is feasible?

It might be ok, I am not sure it is conservative in every situation? Assuming it is, it should likely be the default, right now your MemAlloc default to full.

Thank you for your guidance. I have update the patch according to your comment. @mehdi_amini

Do not set side effects stage for operations where we cannot determine the order of side effects on all values.

Sorry, I don't know how to reply inline comment...
The copy like example read source on stage 0 and write target on stage 1. By using stages, we indicate the sequence in which these two side effects occur. Is it enough for "stage" or we need to provide more exmaple?.

In D156087#4549716, @mehdi_amini wrote:

In D156087#4549709, @cxy-1993 wrote:

Regarding May/Must associated with the effect, I believe you are referring to the relationship between memrefs. If that's the case, we will leave this issue to the pass to invoke alias analysis. If it's not, please let me know.

No I wasn't referring to alias analysis (where "May alias"/"Must alias" is important) but to the read/write effects: how do we model an op that may write the full resource, or may not?
We can say that conservatively we say "write full" when it may do so, but then you can't use this information for kill the liveness of the entire resource.

Thanks for your advice.
The "partial/full region" aspect of the effect is also an important feature in side effect, i have already added them to the member variable of EffectInstance and they can be specified via the 'td' parameter.
Regarding May/Must associated with the effect, I believe you are referring to the relationship between memrefs. If that's the case, we will leave this issue to the pass to invoke alias analysis. If it's not, please let me know.

Update style

Add effectOnFullRegion to side effect and update docs according to commentsx

I agree with your suggestion of making 'stage' as a member variable in the side effect class. I have already updated the code and added the necessary documentation based on your advice. I kindly request you to review it once again. Thank you so much for your valuable comments! @mehdi_amini

Updated the code according to the latest review comments

In D156087#4549079, @mehdi_amini wrote:

I still think documentation is missing, I don't see a definition of "Stage" yet?

Update test

Could you please help me review this piece of code again @mehdi_amini . Also, I am unable to trigger the tests for this commit again. I found online that it's currently only allowed for maintainers to trigger the tests. Could you please help me trigger the tests? Thank you very much for your assistance.

Rename SideEffectOrderAttr -> SideEffectStageAttr

In D156087#4544491, @mehdi_amini wrote:

There is likely some documentation to update.

Also I'm not sure that "index" is expressive enough to carry the intent here. The closest I can relate here is pipeline stages in VLIW for example, so maybe the word "stage" should appear in the API?

Updated the code according to the review comments

Update patch fixes coding style

Update the patch according to the discussion in https://discourse.llvm.org/t/rfc-add-effect-index-in-memroy-effect/72235/4

This is the RFC corresponding to this patch：https://discourse.llvm.org/t/rfc-add-effect-index-in-memroy-effect/72235

Could you please help me push this patch to master @springerm

In D151287#4367113, @springerm wrote:

scf.parallel can have an scf.reduce terminator.
E.g.:

%init = arith.constant 0.0 : f32
scf.parallel (%iv) = (%lb) to (%ub) step (%step) init (%init) -> f32 {
  %elem_to_reduce = load %buffer[%iv] : memref<100xf32>
  scf.reduce(%elem_to_reduce) : f32 {
    ^bb0(%lhs : f32, %rhs: f32):
      %res = arith.addf %lhs, %rhs : f32
      scf.reduce.return %res : f32
  }
}

Update coding style.

As discussed on https://discourse.llvm.org/t/why-scf-forall-op-doesnt-have-regionbranchop-interface/70789/4, these two operation has special terminators.

In D148408#4271629, @springerm wrote:

In D148408#4271164, @cxy-1993 wrote:

This patch solves deallocs placement error, but i still have some questions:

1. 'memref.dealloc' is generated in bufferization process of 'scf.for_all' operation. It seems to me that this placement error is caused by illegal insertion, but expect others to fix it. is this over-coupling?
2. If we accept this approach, any operation contains in terminator region should do same things as tensor.paralle_insert_slice on bufferize, Or should we abstract this behavior as interface or something?

I was just tried to contributing to llvm community, maybe i missed some important rationales. Thank you in advance for any help and explanation.

A bit of background: This bug was caused by our buffer deallocation strategy, which is currently too simple: it always places deallocs at the end of a block, right before the terminator. This worked great, until we added scf.for_all, which was the first time that we had an op with a terminator that does more than just yielding.

You are right, this is a not an ideal solution because the root problem is the original placement of the dealloc. We have plans to revamp the buffer deallocation logic this summer: One-Shot Bufferize would no longer insert any deallocations. This would be done by a separate pass. This change here is the smallest change to fix the issue for now, without writing a bunch of code that will be deleted in a month.

Contributions are always welcome, so please keep them coming :)

This patch solves deallocs placement error, but i still have some questions:

1. 'memref.dealloc' is generated in bufferization process of 'scf.for_all' operation. It seems to me that this placement error is caused by illegal insertion, but expect others to fix it. is this over-coupling?
2. If we accept this approach, any operation contains in terminator region should do same things as tensor.paralle_insert_slice on bufferize, Or should we abstract this behavior as interface or something?

In D147790#4270628, @springerm wrote:

Can you try this patch: https://reviews.llvm.org/D148408. I ran into a similar issue today and I think this should fix it.