- User Since
- Oct 18 2012, 4:53 AM (339 w, 1 d)
Thu, Apr 18
LGTM. Thanks for updating it.
Wed, Apr 17
IIUC freestanding environment should not rely on memcpy being present so my take on it was that by "fixing" freestanding I could have my cake and eat it too.
I think it'd be pretty unpopular with the people I know who use freestanding. They're mostly working on microcontrollers and compiling -Oz so the extra code size would be untenable; they also have memcpy implementations anyway because they use it in their own code.
Tue, Apr 16
I think this needs splitting up further. There are a lot of novelties in the assembly syntax with this new instruction set, and it's made the patch huge.
The code looks reasonable to me, but should be tested.
I haven't changed ComputeValueVTs yet.
Switched to using getPtrExtOrTrunc. I *think* it's right, but there was a certain amount of guesswork on whether it'll actually work for signed pointers.
OK, it's a bit ugly but pretty limited in scope so I think we can live with it. LGTM.
Mon, Apr 15
Have you run make check on this patch?
Excellent, looks good to me then.
I think it would be better to fold this patch in with whatever smallest unit makes it testable. Probably by splitting up the different fixups to go in with the instruction that uses them.
This should probably be combined with the first patch that uses these addressing modes so that it can be tested.
What goes wrong if we don't do this? Does TableGen complain about schedules containing unsupported instructions or something?
This looks good to me.
Could probably also do with some testing in unittests/Support/ARMAttributeParser.cpp and TargetParserTest.cpp.
This needs some tests. I'm also not quite sure when you'd use bare "+fp", if it's the default anyway.
I like the direction of this change, and the details look correct too.
This looks reasonable to me.
Could you add some tests?
Is this coordinated with GCC?
Thanks, *this* one is r358399.
Thanks, it's r358399.
Thanks, it's r358399.
Sun, Mar 24
Looks fine to me.
Fri, Mar 22
Did you look into a scalar variant of the intrinsic call instead? These instructions have non-vector variants (e.g. sqadd s0, s0, s0), and that's actually why the intrinsics exist in the first place. It'd be a shame to always require this extra work.
Mar 12 2019
Thanks for the review. Committed as r355926.
Mar 11 2019
Added context, and moved use of NeedsRegBlock from incorrect patch I'd uploaded elsewhere.
Accidentally had part of the ConsecutiveRegs patch in here.
Now with added context!
Please add context
Mar 5 2019
Feb 8 2019
Feb 7 2019
I was looking at this again and noticed it didn't handle unordered loads & stores properly. It tried to create unordered atomicrmw/cmpxchg, which are illegal and assert. So I updated it to promote unordered to monotonic.
Feb 6 2019
Jan 25 2019
I think this looks pretty reasonable. The penalties are a bit speculative (fairly inevitably since the instructions haven't been created yet), but look sane. If they turn out to be problematic in future we could add some hooks to customize them.
Jan 24 2019
Jan 22 2019
Thanks very much for working on this, those numbers look promising. I had a quick read over, and have a few comments. I haven't looked at the checking logic in detail yet, mostly just the CodeGen part.
Jan 16 2019
Dec 20 2018
Dec 18 2018
Thanks Gerolf. Committed as r349465.
Dec 14 2018
Dec 13 2018
I think the consistency argument is good, so I'm in favour of the structure you chose now. My main issue at the moment is the tests: hard-coding register usage leads to very fragile tests.
I think this looks fine.
update_llc_test_checks is a useful starting point because it generates the right checks to ensure you're checking the whole function, which is inconvenient to do by hand (trying to CHECK-NOT your way to checking the whole function is a bad idea).
Dec 12 2018
LGTM; well spotted!
Dec 11 2018
What does it do with floating-point inputs?
Dec 7 2018
Separated NFC alias analysis move out into https://reviews.llvm.org/D55420, which this change applies on top of.
Dec 4 2018
Maybe explicitly add reviewers?
Dec 3 2018
Great to see those other test changes!
Nov 28 2018
Ah, I see what you mean. It's not pretty, but this updated patch seems to do the trick.
Nov 27 2018
Good idea. I committed a comment in r347653.
Nov 20 2018
The current implementation is the first and is modeled after what sign_extend_inreg does.
Interesting; I (obviously) hadn't considered that interpretation of what's going on, or I'd have mentioned it. I think it just about holds water, so thanks for setting me straight!
Nov 15 2018
This isn't meant to change the semantics of C and C++.
Nov 14 2018
Ok, fair point. If we are going to introduce a new node to fix this issue, could we have a SUBS node that can be glued to the CMOV?
Nov 1 2018
Same as previous patch except the OSLog helpers are moved to libclangAST to respect dependencies.