- User Since
- Jul 12 2017, 1:23 AM (177 w, 3 d)
Wed, Dec 2
Should there be a range limit somewhere in this logic, beyond just 'in the same basic block'? I worry slightly that if there's a very long basic block with two completely separate predicated sections, and they happen to reuse the same constant, then it might not be a win to do this change. Something along the lines of
Tue, Dec 1
Mon, Nov 30
Fri, Nov 27
Fri, Nov 20
Thu, Nov 19
Oct 16 2020
Thanks for the quick review – sorry I wasn't on the ball enough to fold D89547 into this one too...
Oct 14 2020
Thanks for the quick response!
Hi @lebedev.ri , it looks as if this commit is causing an assertion failure compiling the following example code.
void bar(void), baz(unsigned);
Sep 3 2020
Aug 7 2020
Aug 6 2020
Aug 3 2020
Jul 31 2020
Jul 20 2020
Jul 14 2020
Jul 3 2020
@jdoerfert , @clementval : over in D83032 is a polished-up version of the script I used to check where the missing deps needed to go. Might be useful for the next problem of this kind. But I'm not sure who to get to review it; perhaps one of you might look at it?
Jul 2 2020
Jul 1 2020
[facepalm] Thank you. I carefully wrote a revised description, but forgot to upload it to this issue.
Here's a completely different patch, which adds all the missing dependencies on OMP.h.inc in the clang subdirectory in one go.
Jun 30 2020
Cannot you just add a depends for your failing case?
Unfortunately, I had to revert this because it caused a buildbot failure: rG39ea5d74b283d5a42f34b856d22bfaf806a1c907
Jun 29 2020
@clementval , are you happy for me to commit this patch?
Jun 26 2020
I wondered about that. I think it may well mean some of those DEPENDS can be removed, but I'm not sure how to be certain of it :-)
Jun 24 2020
LGTM. Thanks for the fixes.
Jun 23 2020
Updated for @dmgreen's review suggestions, which both seem to work as far as I can see.
It isn't unused – there are still some isel patterns using it, which I haven't touched in this commit.
(Sorry @labrinea – removing you as a reviewer was an unintentional side effect of a bad arc command.)
New version addressing review comments.
OK, fair enough. In that case, please correct my nitpicks and I'll approve the revised patch.
Jun 22 2020
for ARMv7 we only disallowed this pair, and this change removes that restriction. So now, all of them are allowed...
I've made a couple of nitpicks to the patch, but my more general question is: if we think that this pair of coprocessors should be changed from "reject completely" to "accept with a warning", why not all of them? What argument applies to CP10 and CP11 that doesn't apply to all the others, apart from the immediate short-term argument of "this is the one that's currently causing somebody a problem"?
Jun 17 2020
Jun 15 2020
May 28 2020
May 5 2020
Apr 21 2020
Apr 20 2020
I see. In that case I suppose the simplest thing to do is just to explain that in the commit message, along the lines of "D77872 has already added the MC representations of the instructions so that they can be used in code gen; this patch fills in the details needed to make assembly parsing work, and adds tests for asm and disasm'.
The important part of this patch seems to be missing! It says it's adding AArch32 assembly parsing for some new instructions, but I don't see any new Tablegen work that adds the instructions. New mnemonics are added to a couple of exclusion lists in ARMAsmParser, but there's no code in here that makes the mnemonics actually exist in the first place, or that says what their operands should be or what their encodings are – just that small C++ tweak and a load of tests. Where's the rest of it?
Apr 7 2020
Apr 6 2020
Apr 1 2020
I have no thoughts on the patch itself, but the commit message looks quite alarming out of context. Perhaps it should mention that you're doing this specifically for i1 and vectors of i1, and not for bitwise OR of ordinary integers?
Mar 27 2020
Do you understand why all those test outputs have changed as a side effect of this?
Mar 26 2020
Mar 25 2020
Mar 24 2020
Added an entry to the Clang release notes as well.
Mar 23 2020
LGTM, with only one tiny remaining nitpick.