User Details
- User Since
- Oct 19 2012, 12:57 AM (501 w, 21 h)
Today
Yesterday
This looks good to me, but wait to make sure others see it, too.
Wed, May 25
First, I think this is a good idea and can eventually mitigate the general problem of intrinsics vs. instructions in other LLVM passes.
Thu, May 12
Sat, May 7
Fri, May 6
Apr 21 2022
Apr 20 2022
LGTM, thanks!
Apr 19 2022
Looks reasonable to me. Some nits on file naming and definition placement. Thanks!
Apr 14 2022
Thanks @iliya-diyachkov, it looks cleaner, even if still very long. A table generated output will hopefully fix this soon.
Apr 13 2022
Apr 6 2022
Apologies, it seems I was looking into a very old version of the review!
This one still has some unactioned comments...
Thanks!
Apr 5 2022
Seems most (all?) comments were addressed, @arsenm is there anything critical that needs done before the merge of the first batch?
Honestly, I don't see why this commit would be a problem without tests, given there aren't tests for the rest. It seems like an obvious omission to me.
Mar 31 2022
Mar 30 2022
I just applied this to a recent HEAD and got a few warnings. Please make sure there are no new warnings on changes / new files.
Mar 22 2022
My original point was that this change is so small and unrelated to anything at the moment that it would be better for whatever s390x or i386 change that needs it, to introduce it then, not beforehand.
This is already tested by its users (via lookupTarget) in all sorts of places, but not this change in particular, which is the problem.
Mar 17 2022
I'm surprised these tests are passing for you. Perhaps you're not building or running them all.
Mar 11 2022
I don't know enough about your toolchain requirements, but this looks good to me.
Feb 28 2022
With the nit, this looks good to me too, thanks!
Feb 19 2022
I think you forgot to upload the new diff for the GCC comment. It's looking the same as before for me.
Nice catch, thanks!
Feb 10 2022
Than you @SixWeining, the 6 patches have been merged now.
Feb 8 2022
AFAICS, all issues have been fixed and the last contentious was about changing naming on the GNU side, which is completely out of scope in this series.
Sure, no problems. Let's just land all patches at once, in order, after they're all approved.
Sorry, this fell out of my radar. Looks good to me, thanks for the hard work!
Feb 4 2022
Feb 3 2022
Feb 2 2022
I didn't mean the duplicated types/constants would survive into codegen, but I get your meaning, it would have to be commoned up at some point.
I'm still not convinced that these passes are the right way to go, but I won't block if others think it has merit, even if just temporary, until the proper implementation emerges.
@MaskRay last round?
@MaskRay last round?
@MaskRay last round?
@MaskRay last round?
Phab comments are out of place in Subtarget, so I'll let @arsenm approve this one if his comments were addressed.
All comments were addressed and this looks good to go. Thanks!
All comments seem to have been addressed and the patch itself is very mechanical in nature, so LGTM.
This looks good to me as the first patch.
Jan 26 2022
Jan 21 2022
Jan 20 2022
Jan 12 2022
Jan 10 2022
Jan 9 2022
Jan 7 2022
Jan 6 2022
Jan 2 2022
Jan 1 2022
You don't seem to have tests checking if the encoding of the instructions are correct when printing or parsing, leaving the Printer/Parser completely untested.
This is a much larger patch and I don't know the target well enough. I have a few general comments inline but would be nice if someone that knows the target better to have a look?
I'm guessing that the missing bits (ex. frame lowering) will be implemented in following patches.
Same as the others, looks like a standard MC patch. I do have some inline comments, though.
Nit: The name of this patch is still "[2/n]"
Can't see anything wrong with it but I don't know the SPIRV ISA, so...
Hi, this looks good as the first patch. Will keep looking for the others.
Dec 28 2021
Thanks!
Dec 22 2021
I can't see anything wrong with this patch. It seems to have all the bits to start a target in the right places.