- User Since
- Sep 26 2016, 7:58 AM (142 w, 6 h)
Mon, Jun 10
Please add test cases for scale=0 and scale=width as I assume those need special handling (UB right now?).
And if scale=0 and scale=width needs special handling, then I guess scale=1 and scale=width-1 are new boundary values so I maybe it would be nice to have tests for those scales as well.
Looks good to me (matching my assumptions about why prolog/epilog is special).
Just a minor nit inline.
Fri, May 24
Is the intention to change the behavior for prologues as well here? The description mentions that we do a different analysis for the epilogues. But with this patch we no longer ignore register clobbering for "FrameSetup" (collectChangingRegs used to ignore FrameSetup code).
Thu, May 23
Now calling the new helper in DataLayout typeSizeEqualsStoreSize().
Wed, May 22
Add a helper in DataLayout "isTypeStoreSized()" as suggested in the review.
Open for suggestions regarding the name (or is it clear enough what it means?).
I don't know much about compiler-rt, and neither what the procedure is for adding new builtins.
Maybe try to find some more reviewers (someone that has been doing/reviewing this kind of additions before).
Just a minor fix to use "Type *" instead of "auto *".
Mon, May 20
May 17 2019
LGTM! (if all comments from other reviewers has been taken care of) (maybe you should wait another day to see if anyone else object, but I think this patch has been open for a long time so there have been plenty of time for comments already)
May 15 2019
Adding myself as reviewer to remember to look at this patch.
May 14 2019
Sorry. I realize that I've completely forgotten to look at this. I'll try to find some time before the end of the week.
May 13 2019
FYI: I have no further comments.
May 10 2019
May 9 2019
May 8 2019
May 7 2019
May 2 2019
I'm suggesting to replace this with a solution where the subregister name is case sensitive (in MIR parser).
May 1 2019
Apr 29 2019
Apr 28 2019
Address review comments from @spatel (thanks!).
Apr 26 2019
Apr 25 2019
Apr 24 2019
Apr 23 2019
Apr 19 2019
Apr 18 2019
Apr 17 2019
If there still are worries about this, then maybe I can do the visitADD refactoring in a separate patch? And then I can limit this patch to the part where we start to use visitADDLike from visitOR.
Looks generally good. But...
Apr 15 2019
Apr 12 2019
Rebased. Corrected typos in the test cases.
LGTM! (with a nit in the test case)
Apr 11 2019
Updated to use a KnownBits::computeForAddCarry helper. I added a simple
implementation here, but the idea is to replace it by the improved version
from D60522 instead (if we land that patch before this one).
Removed the add_zext_ifpos_vec_splat2 test from test/CodeGen/X86/signbit-shift.ll again (as suggested by @lebedev.ri). That test was only added to demonstrate why add_zext_ifpos_vec_splat gets an extra movdqa with this patch (due to unfortunate reg constraints), but it did not contribute anything new when it comes to testing "signbit-shift".
Apr 10 2019
Simplified based on feedback.
Use KnownBits::computeForAddSub instead, and do it for both ADD* and SUB* nodes.
Apr 9 2019
Added a unittest (by using the AArch64SelectionDAGTest unittest framework).