- User Since
- Oct 9 2015, 4:06 AM (296 w, 1 d)
Thu, May 20
Tue, May 18
Invert the logic of the patch: mem intrinsics shouldn't be unconditionally marked nosync in the first place.
May 11 2021
Fired too quickly, the Attributor actually had a bug and an incorrect lit test. This version should fix that.
Split up the patch as per @reames' request
May 8 2021
May 6 2021
Address review feedback: Adding the required change to FunctionAttrs that @reames pointed out; in the new version, I also made sure to really go through all test cases.
May 5 2021
May 1 2021
I tried my hands at a change in the opposite direction in D101701
Apr 28 2021
Apologies if this is obvious somehow, but could you please point at those "deref" changes? I think I missed that part of the conversation. My hope would be that it could be treated orthogonally to the nofree/nosync question.
Apr 23 2021
Apr 17 2021
I think the crux of the discussion is the second half of this sentence:
Apr 15 2021
Thank you for doing this.
Apr 13 2021
To address this there was an attempt to invert the behavior of convergent attribute in this patch (https://reviews.llvm.org/D69498) then the frontend wouldn't need to generate the attribute everywhere and the optimizer wouldn't need to undo what frontend does. The change in this review doesn't address (2) as far as I can see - it seems it only generalized old convergent semantics to cover the cases with non-uniform CF. I am not clear yet about the details of how and what frontend should generate in IR for this new logic but it looks more complex than before. And if we have to stick to the conservative approach of assuming everything is convergent as it is now this might complicate and slow down the parsing. So I am just checking whether addressing (2) is still feasible with the new approach or it is not a direction we can/should go?
Hi @jholewinski, sorry for missing your comment earlier. It's been a while! I still need to work through the rest of the comments here, but there's a pretty crucial point here that seems to have been missed:
Hi @Anastasia, thank you for your comments. I replied inline, but a high-level point upfront is that in many ways, this patch only exists because HLL don't really have well-defined semantics for convergent operations yet. Most of us have a shared mental model of what they should be for most high-level constructs, but intuition breaks down for the trickier corner cases. I made some proposals in the Khronos Memory Model TSG for how useful semantics could be added to HLL, taking the corner cases into account, but on my end all of this is on hold while I'm on leave.
Apr 7 2021
The change makes sense to me. Does this need a documentation update?
Apr 4 2021
Apr 1 2021
I think this requires a lot more thought.
Dec 29 2020
Dec 27 2020
Dec 23 2020
Dec 21 2020
Also, I did not suggest to change the semantics without an RFC and everything else that is needed. You suggest otherwise in your response. I did propose alternatives to fix your bug, but most of my responses show how the restrictions you want to put in place are making (potentially existing) optimizations invalid.
The thing is, in our view (aqjune, nlopes, and me) you *are* changing the semantics by allowing the lifetime intrinsics to be used on things like "malloc". From all we can tell, the intrinsics were never meant to be used with anything but alloca, so for all intents and purposes, currently, they only support alloca.
Dec 14 2020
Thanks for expanding on this documentation.
Dec 9 2020
Remove HandleWrapperFor which got de facto superseded by SsaContext
(subsequent patch). Update comment slightly to match.
Seems trivial enough, and there doesn't really seem to be an owner of this code...
- rebase on factored-out Handle infrastructure
- rebase on factored-out Handle infrastructure
Nov 26 2020
Nov 25 2020
(comparison being a total ordering) /// This avoids having to add asserts the comparison operators that the states /// are valid and users can test for validity of the cost explicitly.
Counterpoint: The asserts cost nothing in a release build, and for software maintainability, having the asserts once in the comparison operator is better than having them at every callsite. Users of this class will forget to put them there.
After our internal discussion of this fizzled out, I am no longer actually sure whether this analysis is correct.
Nov 9 2020
It seems the MTYPE numbering was changed on gfx9, reflect that in the comment.
Nov 6 2020
Nov 5 2020
This makes sense to me.
Nov 4 2020
This seems reasonable to me.
LGTM modulo the inline comment.
Does this mean it's time to remove the lo-hi syntax altogether?
Oct 30 2020
I'm going to follow up with another RFC about this on llvm-dev.
Address the comments from @jlebar that I indicate I'd address,
except for changes affecting the Verifier -- I'll do those later.
Oct 27 2020
Oct 26 2020
Overall, I think the policy change proposed here isn't entirely unreasonable, but I do think it needs to be treated as a change of policy. Not everybody is necessarily aware of a 4 year old email thread; heck, I've been working on upstream LLVM and frontends for 5 years and I wasn't aware of this supposed policy where in reality, things weren't actually done according to what's written in the policy documents. If I don't know this after 5 years, how can anybody joining in the last 4 years possibly be aware other than through mere chance. What's written in the documents needs to matter, if only as part of making LLVM a welcoming and inclusive community. Hidden tribal knowledge is the opposite of those ideals.
Oct 24 2020
I replied on llvm-dev.
Considering that you are doing this in response to D83088, I think we have pre-existing evidence that this isn't workable as-is. There has to be some obligation on the people "thinking there should be more review" to actually engage in review. In particular, if they have previously demonstrated that they cannot be relied on for this, then the rule should not apply.
Oct 23 2020
Hi Mehdi, this is not an appropriate place for this discussion. Yes, we have a general rule that patches can be reverted if they're obviously broken (e.g. build bot problems) or clearly violate some other standard. This is a good rule, but it doesn't apply here. If you think it does, please state your case in the email thread that I've started on llvm-dev for this very purpose. Just one thing:
Oct 22 2020
Oh, and on the change itself: I'm not familiar enough with the attributor framework to judge the implementation, but the described reasoning (being able to make deductions from indirect calls) is sound.
If we're nitpicking over such details, I would point out that it would be better to structure the whole thing differently, with a single legalizer class in which the insertAcquire etc. methods have different paths based on the hardware generation.
I noticed what I believe is one more correctness issues, plus a few assorted comments.
Oct 21 2020
David, I don't think this is appropriate here. Let's take the discussion to llvm-dev.
Oct 20 2020
Oct 19 2020
LGTM modulo that inline question.
This is fine for graphics.
Adding writelane absolutely makes sense to me, but:
Duplicate of D89596?