- User Since
- Oct 15 2012, 2:12 PM (282 w, 4 d)
Wed, Mar 14
This looks like what we talked about, though I've not looked any more closely than that :)
I'm fine adding these. I'd appreciate not breaking the go bindings so updating those would be great if you can.
Tue, Mar 13
Fri, Mar 9
It's be great if you wouldn't mind. Still working on getting you a test
Thu, Mar 8
Some inline comments.
One inline grammar-o.
Wed, Mar 7
Tue, Mar 6
You can probably just commit this sort of thing for xray.
Fri, Mar 2
Thu, Mar 1
Wed, Feb 28
Seems reasonable to me.
LGTM. Feel free to just add any patches like this in the future.
Using sections as references is, in general, a good idea when we can so I'd rather not tie that to whether or not we emit certain sections. If you don't mind reverting that aspect of the change I'd appreciate it and we can get that in via some other preference or a strict requirement via the asm printer as we're planning on doing for the rest of the nvptx section emission.
Fri, Feb 23
Start of some comment requests.
I spoke with Ian about it and he said the whitelist fix is also targeted at 1.9.5.
Nah, don't worry about it. Thanks :)
Thu, Feb 22
Some inline comments - I think this looks ok in general, but I'm curious about the answers to the questions/documentation bits inline.
LGTM. Thanks for the cleanup.
TIL we accept regexes here.
Feel free to commit anything like this in the future and we'll do post-commit review on it.
Wed, Feb 21
Tue, Feb 20
Sun, Feb 18
Fri, Feb 16
We have some testcases like this, but not a lot. We could add one - also we should probably not assemble if we're using cannonlake as well. The aarch64 backend has some examples of llvm-mc tests in this case.
I'm really not a fan in general of this path - can we step back and figure out what we'd like to do with this first? I don't really agree with the need for require-vector-width in the first place.
One inline typo, otherwise fix up what Paul asked for and LGTM.
Leaving it as an IR only check seems like the right thing to do. We definitely don't want to encourage people to have .cc -> .s checks.
Thu, Feb 15
I don't know the code well enough here to just say yes or no, I'd be fine with you doing post commit review here since this is reasonably well tested and nothing else has broken.
Seems ok here.
Feb 14 2018
Feb 13 2018
Another thought here is getting that flag put into the default set that's allowed in go?
Feb 12 2018
LGTM for now, but the set of function attributes being passed into the subtarget is getting a little awkward so we should come up with some better way to encompass them.
(If you need someone to commit this let me know)
Feb 11 2018
Feb 9 2018
Feb 8 2018
Huge fan of refactorings :)
I've glanced at this and didn't notice anything. I think any problems can be fixed in post-commit review if necessary.
LGTM with an inline comment.
Feb 6 2018
Should we just pick a name and go with it?
Feb 5 2018
Feb 4 2018
Feb 2 2018
A few comments inline.
Feb 1 2018
Jan 31 2018
One inline comment then LGTM.
Jan 30 2018
Jan 29 2018
Awesome. Do you have commit ability or do you need me to commit for you?
SGTM. Did you try running it on a file to double check that you didn't change anything? :)
(Was going to let hfinkel review since he'd started on it. efriedma also had said something).
Jan 26 2018
Seems reasonable? Not a fan of the ifdef, but I also don't know what changed in 10.11 here. If it's wrong we can work around it later.
(Also sorry, I thought I'd hit submit on my comments a few days ago, but apparently hit close tab before I'd gotten that far)
Jan 23 2018
I'm ok with this, might want to wait on efriedma and hfinkel to take a look.
Jan 18 2018
In general some of the code generation is a bit confusing and not directly extensible to, say "prefer 128 bits", or even feels a bit awkward around what ISA to use rather than what vector length to do our computation in.
In general, I think this patch needs a lot more description and possibly breaking up. For now a few questions:
Let me know if you need this committed.
I just noticed this was never committed. I found another one and committed it here:
Jan 16 2018
Jan 12 2018
Also the register saves strike me as a somewhat odd optimization - you could also have just thrown this into SAVE_REGISTERS I think?