User Details
- User Since
- Dec 17 2012, 10:03 AM (278 w, 5 d)
Mar 22 2018
Do you have any comments on the regbankselect part?
Mar 19 2018
Mar 14 2018
Hi Krzysztof,
FWIW, LGTM now.
Mar 5 2018
Hi Krzysztof,
Feb 27 2018
Hi Reid,
Hi Konstantin,
The %1 assignment is dead but not marked as such. I'm not 100% sure whether it is allowed to omit dead flags.
Feb 26 2018
Any chance you could add a test case?
Feb 21 2018
Hi Geoff,
Feb 16 2018
FYI, I've revert the original commit in r325421.
Hi Aditya,
Thanks all for the great feedbacks!
Hi Amara,
Feb 13 2018
(though it may require the interface to be changed slightly since we'll need the opcode)
Feb 12 2018
Feb 9 2018
Hi Geoff,
Feb 8 2018
BTW, while writing the RFC, I realized that we could potentially generated incorrect code if we were silently downgrade a post-r317488 bitcode with a pre-r317488 compiler. (I.e., running fast math optimizations whereas we only wanted reassoc)
I think we have to create a new version of the IR since the bits changed meaning (we can't just flip 'on' new bits).
Hi Sanjay,
For the review itself, I leave this to @aprantl. I was just providing insights on the shrink-wrapping question :).
Feb 6 2018
LGTM
Feb 5 2018
Hi Roman.
Feb 2 2018
LGTM
LGTM
One question below.
LGTM, coordinate with the x86 changes and you should be good to go!
Feb 1 2018
While I was here, I notice that selectCopy was not using getRegClassForTypeOnBank. Could you double check this is intended?
LGTM with the nitpicks mentioned by Krzysztof.
Jan 31 2018
LGTM
Jan 30 2018
Just two remarks:
- So far shrink-wrapping was solely an analysis pass, i.e., it didn't modify the input code
- Ideally I'd like we postpone creating new blocks until we decided where we are going to insert the code, otherwise we'll end up with blocks to clean-up later on
Ah makes sense.
Let's leave the patch as is.
Jan 29 2018
Alright, LGTM then.
Please also patches the two other places I mentioned assuming you can have a test case for them.
Got a chat with Roman off-line and turns out the problem I mentioned is worked around by everything single target, so indeed no test cases!
LGTM then
Couple of nits on the test case.
Jan 26 2018
Basically, I am fine either way (with or without the cast), but we would need to be consistent.
Out-of-curiosity do you actually see codegen differences with that patch?
LGTM
Jan 25 2018
LGTM with couple of nits below.
LGTM
Hi Carlos,
Jan 23 2018
LGTM too. The code is probably simpler to understand like this and if compile-time turns out to be a problem we can revisit.
I wonder though if removing this copies upfront wouldn't have helped tablegen current matching table (because it does not look through COPYs right now).
Sounds like a good thing to do.
Jan 22 2018
But that does not imply that there is no correspondence to user code before the frame setup is complete.
Jan 19 2018
Should I submit autogenerated tests that are not FP/G_COPY/G_TRUNC related, but that still need to be updated for D37775 as a separate patch? Should I have a review for it or can I just submit?
Hi Alexander,
Jan 18 2018
Mostly looks good to me.
We probably want to make two separate patches:
- One with the pattern matching capabilities
- One with the new pass
Hi David,
Could you upload the patch with the full context?
Jan 17 2018
Hi Geoff,
Jan 12 2018
Looks mostly good to me.
I don't expect you have a test case that exposed the problem, right?
(In particular for Cyclone at least WriteAdr is the same as WriteI).
Jan 10 2018
Hi Diego,
Hi Marina,
Jan 9 2018
Hi Volkan,
Jan 5 2018
Jan 4 2018
Dec 20 2017
How about checking for EnableGlobalISelAbort.getNumOccurences() > 0 to check for an command line override, otherwise doing what the target wants via the isGlobalISelAbortEnabled() override?
Sounds like this option doesn’t use the fallback path.
I thought it did.
Something to double check.
Dec 19 2017
One more comment: Do you have an idea how we could make sure that the combiners are making progress?
Basically, I would like we think about two cases that bit us in the past:
- Two combines undo each others
- Two combines keep piling up (like expand op1 into op2 & op3 and expand op2 into op1 & op3 would keep having the IR growing)
Hi Aditya,
Dec 18 2017
Sounds like this option doesn’t use the fallback path.
I thought it did.
Something to double check.
Hi Amara,
Dec 15 2017
Dec 14 2017
Hi Philip,
Dec 13 2017
LGTM