- User Since
- Jan 26 2016, 2:17 AM (335 w, 3 d)
Tue, Jun 14
Yay, one more postinc, LGTM.
Tue, Jun 7
I also think this must be a binary op.
Thanks for fixing, LGTM.
May 30 2022
May 17 2022
May 13 2022
Ok, cheers, LGTM
Not that easy to spot that this is an improvement from these test changes, but I think I see a few, and the idea makes sense, so LGTM.
Looks like a good fix. One quick question: I see that some CPUs have FeatureAlternateSExtLoadCVTF32Pattern set. Is that something we want too?
May 6 2022
I haven't addressed feedback yet, but have just added a few things to help with the discussion where this belongs.
This now also prints the number of bytes per instruction, so for thumb it now distinguishes between the narrow/wide encodings, and I have also added printing a histogram of immediates. This will look now look like this:
Thanks all for looking at this!
May 5 2022
Apr 14 2022
Thanks for explaining! Makes sense, LGTM.
Apr 13 2022
Thanks for the introduction to perfect shuffles! :-)
That's quite a lot change in the test cases. It's easy to see that the smaller ones are improvements. For the bigger changes that isn't that obvious. But I trust you have run numbers and this is overall better. So LGTM, let's give this a try.
Apr 4 2022
Looks like a good one to me.
Mar 31 2022
Thanks for fixing. LGTM too.
Mar 25 2022
It's looking good to me too, just a last round of nits (inlined) and one question just to double check: is the SPEC score the same? I.e., do we still specialise what we want to specialise?
Mar 15 2022
Mar 14 2022
Mar 11 2022
Mar 10 2022
Sorry for the delay. I need to look a bit more at this, but I added some first thoughts inline.
Mar 3 2022
Looks like a nice change to me.
Thanks, looks like a good change to me.
Mar 2 2022
Looks like a good refactoring, but needs a clang-format, and have added one question inline.
Feb 22 2022
Sorry for the delay. First, about a NFC and some refactoring, can the reshuffle of ArgInfo and SpecialisationInfo and the changes in the Solver functions be an NFC change perhaps?
Thanks, looks good!
Feb 21 2022
Ah, sorry, forgot about this. Can you upload the patch with some more context please so I can have a quick look again?
Feb 17 2022
Ok, thanks, looks like a nice patch to me.
Feb 16 2022
Clarifying my previous comment a bit more:
There is quite a lot to unpack here:
Feb 15 2022
Yeah, nice patch, thanks.
Feb 10 2022
Feb 9 2022
Gentle ping. Any further thoughts on this?
Feb 2 2022
Feb 1 2022
Agreed, lazily removing the instructions in this way is what I was expecting.
Jan 31 2022
Okay, thanks, that sounds like a good plan!
Jan 27 2022
Jan 26 2022
Hi @asbirlea, thanks for checking, much appreciated! About:
Jan 25 2022
Looks like a good change to me.
Jan 24 2022
I did leave the legacy behavior as an option since a) loop-flatten stops triggering if I extend due to weirdly specific pattern matching I didn't understand and b) we could reasonably use the mode if we'd externally established a lock of overflow.
Jan 20 2022
Hey, it's me again. :)
Any plans to pick this up, shall we get this in?
Thank you for resolving all the issues raised! Could you rebase this patch on ToT? (to include the MSSA update and the move to LPM1?)
Jan 19 2022
With your help, we have fixed quite a few things, like updating the MemorySSA state and moving it a LoopManager (and fixing a performance regression related to that).
@dmgreen : You reverted "rG86825fc2fb36: [LoopFlatten] Move it to a LoopPassManager" because of a downstream regression. With D116660 now accepted/committed, that unblocks this change. This should address this regression, so are you happy with this?
Jan 18 2022
Sorry, my bad, should be fixed now.
Thanks for the review, sorry for the delay.
Addressed comments and have added that to the LPM.
Jan 12 2022
Jan 6 2022
I somehow missed the failing regression test.
Turns out I do need to check AR.MSSA for passing on the MSSA object (or a nullptr).
Jan 5 2022
slightly less & more straight-forward code?
I decided to do the updating of MSSA separately in D116660 (and will remove it from here).
Ah, forgot to mention tests. Without the update MSSAU->removeEdge(), I was expecting AR.MSSA->verifyMemorySSA() to run in an assert (with the expensive checks enabled in my build), but somewhat surprisingly to me it didn't.
So not sure yet if we need tests for this, but am looking into this.
Jan 4 2022
Thanks for the pointers! I have added a PhaseOrdering test, while I now look into updating MSSA.
I think all points have been addressed, so am looking for an LGTM .... anyone interested? :-)
Dec 31 2021
Update of the tests after D110057.
Dec 30 2021
With D110057 committed, is this now good to go too?