Very nice – it hadn't even occurred to me that you might be able to find the branch-target operand automatically as well as identifying Arm/Thumb.

LGTM, but to show willing I've spotted a typo in a comment :-)

Looks nicer now, thanks!

Oh yes, I must have seen if Rn == '1101' && W == '1' and absentmindedly interpreted the W as a write to memory, not a write back to the register.

I was lucky to notice it at all – I only spotted it because one of the test changes in the previous version of the patch did lift an increment of SP past two load instructions. As best I could tell it wasn't harmful in that particular case, but that clued me in to look more closely and see whether it could have been harmful in another case.

What happens about a case like the following:

mov r0, sp
ldrd r1, r2, [sp]
ldr r3, [r0]
add sp, sp, #8

In this case, the LDR of r3 is not directly reading or modifying SP, but it is nonetheless dependent on SP not having been incremented yet, because it's accessing the stack slot that becomes invalid as soon as SP is increased. I don't see any check for that situation here.

LGTM. Thanks!

The table-lookup strategy seems like overkill to me. As far as I can see, the integer mapping required is: 0→3, 3→2. 2→1, 1→0. In other words, all four input values (that can be handled at all) are just reduced by 1, mod 4. So instead of (147 >> (value << 1)) & 3, you could compute (value - 1) & 3, surely more cheaply.

Ahem. Now actually tested, and with the necessary extra enum definition.

You've got a typo in the subject line :-) v2f64, not v2f62.

Thanks. LGTM now.

That seems like a reasonable precaution to me, yes – with only one VPR, there's only one previous value that it might be of obviously low cost to VPNOT.

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

Thanks for the quick review – sorry I wasn't on the ball enough to fold D89547 into this one too...

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);

This discussion seems to have wound down. I'll land this patch tomorrow on the strength of @LukeGeeson's review, unless you have strong objections, @jfb?

In D85009#2187643, @jfb wrote:

Language-wise I think https://wg21.link/p1467 is where C++ is going, and C is taking a similar approach.

In D85009#2187621, @jfb wrote:

You mean: only aarch64 backend supports lowering bfloat16 vectors at the moment?

In D85009#2187549, @jfb wrote:

Is that true of all vector bfloat implementations? It seems like arithmetic on these types is something implementations would likely support.

@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?

In D82659#2125618, @clementval wrote:

LGTM. So later the DEPENDS omp_gen that are in clang subdirectories could be removed right?

[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.

Cannot you just add a depends for your failing case?

Unfortunately, I had to revert this because it caused a buildbot failure: rG39ea5d74b283d5a42f34b856d22bfaf806a1c907

@clementval , are you happy for me to commit this patch?

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 :-)

LGTM. Thanks for the fixes.

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.

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"?