- User Since
- Oct 19 2012, 12:57 AM (279 w, 1 d)
Thanks @MatzeB, apart from the missing pipe in the RUN line, I'm happy when you are.
Mon, Feb 19
Below are some inline reviews, but overall, it's looking good. Please update to the current trunk and check the GCC behaviour on your kits to see if they do the right thing (and what arch names they use), so we don't deviate from the norm.
Sat, Feb 17
Fri, Feb 16
Thu, Feb 15
I think this patch should be merged with D43316 as a first example, and then used in D43319 as a proper example. Once D43316 is approved, D43319 should wait until a few green builds are out on the LNT bots to go, once approved.
Tue, Feb 13
Mon, Feb 12
This goes against the documentation, which only supports sN:
For M-class, "invalid instruction" is indeed more accurate than requires NEON. We don't want people to think they can add "-mfpu=neon" or ".fpu neon" to an M-class to force assembling that code.
Sorry, I missed a lot of reviews this year (mail issues). The patch LGTM with the nit.
Fri, Feb 9
All of the thumb/arm changes I think shouldn't be removed. The main thing is that we can know that stage 1 clang can be compiled as arm/thumb and it runs on all the tests without failure.
Jan 3 2018
Same comment as @fhahn :)
Jan 2 2018
This may need special guard with fast-math flags (precision, etc), no?
Dec 6 2017
I'd wait someone from Mips to review the patch before committing, as the messages change a lot and I have no idea how valid they are.
There are too many redundant tests. I'd keep one for each size/type on both boundaries (0, 31) and add negative tests to check that you can't zip predicate registers with data registers, register numbers out of bounds, different number of registers (2, 4), etc.
Nov 26 2017
I think this change goes against the recent trend of printing multiple diagnostics, instead of a huge dump that will certainly be inaccurate at least some times.
Nov 22 2017
Nov 21 2017
Ok, with the updated comment, LGTM. Thanks!
Sorry, fell through. No brainer, LGTM. Thanks!
Nov 20 2017
Wasn't that the mess about -mfpu=softfp?
Nov 19 2017
Great, LGTM now, thanks!
Nov 18 2017
@olista01 is working in this area...
I finished my review, and apart from my two final comments, everything looks fine.
Nov 16 2017
Is the T constraint only used for movs?
Nov 15 2017
Nov 14 2017
Right, ok, you'll reuse the parser with a different kind later, that makes sense and the code is cleaner this way.
Nov 13 2017
Nov 10 2017
I'v looked everywhere and "psr" is not even pre-UAL. Please, fix the user code instead.
Piggyback on @gberry's review, I can't see anything wrong with it, LGTM. Thanks!
Is the VEDataVectorRegister` to SVERegister change meaningful?
The change itself looks good, I only have one question to clarify a part of the code, but otherwise seems fine.
Just a nit, why did you call it rGPR2? If this is a noPC/noSP class, than a longer but more meaningful name like rGPRnopcsp would be preferable.
For reference, we tend not to add undocumented stuff. We only add GNU extensions when it's simple and necessary. We specifically don't add things that are implemented in GCC source code or test-cases, as that brings us bug-for-bug emulation, which we purposefully won't do.
Nov 8 2017
Much better indeed! I agree with Florian on the additional "bad arguments" tests. Thanks!
Nov 7 2017
Nov 6 2017
LGTM, without the extra whitespace change. Thanks!
Excellent, ok, this patch is looking good to me now. How does this fit into the whole story?
Nov 3 2017
This looks good to me, thanks!
Two nits, and I'm happy. :)
Nov 2 2017
Ha, I see. That was your commit trying to reorder things. :) LGTM.
Why did they change in the first place?
After looking at your PDF, it sounds as though you don't need the while loop at all.
Nov 1 2017
Small nit, but overall, looks good to me.
Apart from the one comment about identifying the register as SVE specific, the patch looks good.
Oct 31 2017
This one is trivial, I agree.
Yes, this change makes sense to me.
Oct 30 2017
So, this is an interesting change, on the TODO list for a while, but the code and the tests are changing too much in areas where not clearly related.