- User Since
- Mar 29 2023, 8:13 AM (35 w, 2 d)
Sep 29 2023
Sep 21 2023
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
Sep 19 2023
Sep 16 2023
@rriddle ping :) Could you please spare some time to review this patch, thank you!
Aug 21 2023
@rriddle Please help me review this piece of code. Thank you for your time. :)
Aug 12 2023
@rriddle Ping :)
Aug 10 2023
Aug 7 2023
Aug 2 2023
Ping @rriddle Could you please help me review this piece of code? Thank you! :)
Aug 1 2023
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
Update patch according to review comments
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?.
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.
Add effectOnFullRegion to side effect and update docs according to commentsx
Jul 31 2023
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
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.
Jul 29 2023
Rename SideEffectOrderAttr -> SideEffectStageAttr
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
Jul 24 2023
This is the RFC corresponding to this patch：https://discourse.llvm.org/t/rfc-add-effect-index-in-memroy-effect/72235
May 24 2023
Could you please help me push this patch to master @springerm
May 23 2023
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.
Apr 15 2023
This patch solves deallocs placement error, but i still have some questions:
- '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?
- 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?