- User Since
- Oct 9 2015, 4:06 AM (275 w, 5 d)
Tue, Dec 29
Sun, Dec 27
Wed, Dec 23
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?
That was very long ago. My best guess would be "oversight".
Oct 15 2020
Oct 14 2020
Please make sure you give this a reasonable amount of testing, but it totally makes sense to me.
How about splitting this change into two parts? Limit this change to generating mixes of 64- and 32-bit moves, and then separately consider what to do about those moves of registers where subregisters are overwritten immediately.
Oct 12 2020
Why does this require a commit upstream?
Looks reasonable to me.
I agree with Matt here. You should be able to do experiments locally. Perhaps sinking should be disabled entirely, or perhaps sinking should be improved to take register liveness into account.
I don't understand how this would help with shader replacement. Can you please explain how you plan to use this?
Oct 7 2020
Oct 5 2020
Oct 1 2020
Sep 30 2020
I'm not trying to fully solve the live range splitting problem greedy regalloc hits. I'm trying to eliminate the isBasicBlockPrologue concept that fastregalloc trips over when inserting spills at the beginning of the block
It's unclear to me what this is trying to achieve. If it is to prevent
bb: <-- reload inserted here during live range splitting $exec = S_OR_B64 $exec, %other ... rest of code ...
... then this change only replaces it by:
bb: <-- reload inserted here during live range splitting $exec = S_OR_B64_term $exec, %other // fallthrough
Hi Carl, what's the status of this?