- User Since
- Oct 23 2013, 6:32 PM (246 w, 6 d)
Thu, Jul 5
Sorry, thought I'd reviewed this. I apparently didn't hit submit.
That was indeed what I was missing. Thanks for the clarification.
Wed, Jul 4
I may be missing something but isn't this a pessimization for the case where the mask is an immediate that can be directly encoded into the and instruction? Why should we prefer dual shifts over something like an AND RAX, 0xfff?
Wed, Jun 27
ping! response needed.
Wed, Jun 20
Sorry for being late to the party, but a couple of optional post commit style comments for possible follow up. Nothing major, just ideas on how to share code and reduce a possible ordering sensitivity.
Tue, Jun 19
I went ahead and submitted the workaround, but I want to record one other idea I tried. It didn't work out, but I'm not sure if that's due to something fundamental, or if I just had a non-obvious bug.
Jun 13 2018
Thinking about this further, I see another opportunity for a follow on improvement, sketched below in comments.
LGTM w/comments addressed before landing.
Current patch LGTM.
Jun 12 2018
Quentin, I read over your suggested approach, but honestly, I didn't entirely follow. Rather than going through another round with an approach that I clearly don't understand, I'd like to go with a quick and dirty hack for the moment if you don't mind. Are you okay with us solving this just for STATEPOINTs at the moment by essentially just disabling remats for STATEPOINT uses?
Jun 11 2018
May 31 2018
This appears to have landed without review. What's going on here?
May 21 2018
Minor code/style comments only. Leaving the broader discussion to engaged parties.
May 17 2018
Seems reasonable, go for it.
LGTM w/minor changes
May 10 2018
May 9 2018
Hm, I hadn't been aware of that myself. Will have to take a look.
Specifically NOT okay to land as is. The quadratic complexity concern is a real one and the window chosen here is very likely too large.
LGTM w/comments addressed before submission.
Ok, I think you missed my point in the other review. The entire idea was to have something which was so obviously correct it didn't need further review. Specifically, to avoid waiting for me or anyone else to review it. Unfortunately, we're now in the worst of both worlds: this change does not appear to be fully NFC and we had a long review delay.
May 4 2018
Committed revision 331557.
I thought about this approach and I agree it's interesting. It does have one serious problem I haven't quite figured out yet though which is that hoisting above the guard changes placement in the final lowered IR. If we'd left it where it was, we might have been able to *sink* the instruction into a block with the only user.
May 1 2018
Good point. I agree this works and likely forms a good basis for what the errno case.
The assertion is checking the a value is not live-in to the entry block unless it is a global. That assert is correct. From your test case, it looks like you've actually found a place where we recurse on a non-local search we shouldn't have. (i.e. a local add with two constant operands should never trigger the backwards walk.)
Apr 30 2018
You'll need something a bit stronger than an aliasing property. You're going to specifically need to model *what* is written to errno. At the moment, we don't have a good way to model a function which writes only to one specific global (errno) or to model what those writes are.
This patch makes no sense. The LangRef text is completely reasonable as far as I can tell. What is your actual intent here? What problem are you trying to solve?
Apr 27 2018
Apr 10 2018
I am concerned that you doing too many things in one patch and that the mix of refactoring and functional changes may contain hard to spot bugs. In particular, the absence of hasValue checks in various updated logic worries me.
Mar 29 2018
Mar 23 2018
Mar 20 2018
Mar 16 2018
Fix Anna's comments.
This is a good idea. I'd copied the basic structure of this from the mem deref printer, but I like the annotation version better. Mind if I submit this one (so that I can start getting some basic testing around this code), and then return to the annotation as a follow on? I'll change over mem-deref while I'm at it.
Mar 15 2018
Note: The variant I committed turned out to be less powerful in one particular case than the reviewed code. I spotted that after submission and fixed it in revision 327655. The fix also improves the invariant.start case which had an analogous problem.
Mar 14 2018
It was pointed out that upstream support for dumping the whole module already exists. It's spelled as: -print-after-all -print-module-scope
address Max's comment. I didn't manage to actual test the multiple latch case through LICM though, see attempt and comment.
Mar 9 2018
address Anna's comments
Clarify a couple of comments.
Mar 8 2018
Feb 23 2018
Feb 17 2018
LGTM provided that the backend changes to take the min of the two have already landed.
Feb 15 2018
Just to second this, I think there's a lot of potential gain to be had by being able to select cold blocks for size and hot blocks for speed. As stated, definitely off topic for this review though!
Feb 2 2018
Jan 29 2018
Is it worth considering doing this as a late peephole? (i.e. after register allocation if the register happens to be appropriate?)
Jan 22 2018
I generally like the direction of the API. Again, not really a qualified reviewer, but I added a bunch of small comments on readability of the API choices from a non-expert. Making this stuff more discoverable would be a major plus and this feels like a potential major step in that direction.
Not really a qualified reviewer for this, but I'll comment this reads more cleanly to someone unfamiliar with the details of the code.
Jan 20 2018
Also, LGTM since I did have time to look through this time. :)
In a case like this where I only had minor comments, please don't wait for me. If davade things this is good to go, run with it. I tend to be very busy and blocking anyone's forward progress on my availability is a bad idea.
Jan 18 2018
Jan 17 2018
Bunch of minor comments, but once those addressed, likely good to go.
Marking to get off my queue.