LGTM

The ACLE has been updated and a new version with change included will be released soon.

I am discussing this with our GCC team as we would like both Clang/GCC implementation to be the same. But you're right that _f16 looks like to be the more consistent choice. I will let you know as soon I know more.

Thanks, LGTM

LGTM, with one nit inline.
Perhaps wait a day in case someone else has comments.

Thanks Eli, also for your thoughts on this:

Looks okay to me, with one nit inline.

It probably should be shouldExpandShift or something?

Thanks for reviewing!

Maybe put the tests into test/CodeGen/ARM/subtarget-no-movt.ll? We have some similar tests there from https://reviews.llvm.org/D23090 .

Addressed comments.

Thanks, LGTM

Was just getting up to speed with this again; just a first round of nits before I start looking at the actual functional change.

Many thanks for your speedy responses and for fixing this!

Many thanks for fixing this. I am going to look at this tomorrow morning.

Hello. I am investigating a crash and this assertion failure on AArch64:

LGTM

The AArch64 side LGTM. Also adding Oliver and Sjoerd as they worked on FP16 side as well, in case they have any thoughts.

LGTM

Thanks, looks okay to me. Perhaps wait a day with committing to give people one more chance to comment.

Now that we have MIR tests, do we still need the added .ll test?

This looks fine to me, but it's the first time I look at this file and table. Perhaps @dmgreen or @efriedma can have a look too.

Thanks for taking a look!

Thanks!

friendly ping

Bail earlier if minsize or hwdiv is not enabled.

Ok, nice, looks like we actually agree: we want this to be errors, not warnings. Perhaps I conflated things too, and this was likely caused for practical reasons: how do we get all tests error free so that we the we can enable the error checking?

And I do remember a possibly valid use case for emitting a diagnostic. I've seen quite a few tests with names like test-pr12345.ll, that were reproducers of compiler crashes, but only contained CHECK-LABELS. I called them dubious tests before (but we don't want to use that term), because these tests are happy when a functions compiles, but don't tests anything whereas they probably should. There are probably a few exceptions where only CHECK-LABELs are okay, so I think a warning along the lines of "you're only checking labels, is that what you really want?" should be fair?

So, I'm not convinced that we can have a diagnostic for your case 3 that would really be meaningful and useful.

Looks like we are unhappy with FileCheck for many and different reasons:

1. we want to error for overlapping dag checks,
2. we want to error for the example in this diff: 1 but not all prefixes are checked,
3. we want a diagnostic for my motivating example in D53710: only labels are checked (check-prefixes are renamed/refactored/misspelled, but not the tags in tests accordingly)
4. CHECK-LABEL is confusing and probably mostly misunderstood

Sounds like that could be an error, rather than a warning, perhaps?

• Don't create div libcalls in ARM mode,
• Bail for SREM instructions. This wasn't checked, and resulted in a crash. Thanks to @dmgreen for pointing this out. Will follow up on this.

fyi: as a first exercise, I found some (real) issues and fixed/committed them in https://reviews.llvm.org/rL347487

LGTM

I decided to continue here, and implemented the diagnostic that Paul suggested in the other ticket, because it's small and a nice demonstrator for this new option:

This is small enough that I'd prefer to see it incorporated into the patch with the "real" warning, rather than have this temporary placeholder diagnostic.

Is the purpose of this warning purely for testing your -warn implementation?

Thanks for reviewing!

• removed the unnecessary assert
• added support for ARM mode, and tests.

Thanks for taking a look!

I'm back :-) Catching up on the last messages.... I will post a patch for review soon that adds an option -warn to FileCheck, and will do some initial plumbing for that. If I understood correctly, it looks like we think that is best. After that initial plumbing, I will then return to this patch. Is that a plan?

While looking what X86 as doing (and why it was doing the right thing), I noticed that I should just implement TargetLowering::BuildSDIVPow2. So that's what I am doing now, and thus it's no longer a DAGCombine change but just an ARM one.
Different tests and targets have been added, and a check for larger constants.

Thanks, looks okay to me.

A little bit of progress:

Thanks for commenting!
Yes, this should save 2 bytes.
I will check tomorrow:

• about the SDIV legality, and where exactly the SDIV is expanded when it is not supported in HW. I see that sdiv is expanded very early if it is not available, immediately after the initial SDAG, and this code in visitSDIVLike does not get triggered. But again, will double check where exactly the magic happens. I've noticed that at least some tests are in place for other targets: the minsize check is no, the !VT.isVector() is required for some X86 and AArch64 tests, and without it the (knock on) effect is that the vector operations get scalarized and these tests fail, but I don't think this is related the legality of SDIV.
• I missed this: "On Thumbv8, if you're dividing by a power of two greater than 128, I think you save zero byte", so will address that.
• what exactly the benefit is on some other targets.

Just a very quick first pass....some nitpicks:

Sorry, just a quick message: I'm a few days out of office, and will get back to this end of this week.

Agreed with all previous comments! And yes, I messed up my last lit test example :-( and I looked at yours and Paul's, and agree. But anyway, we have a plan...

Looks like a straightforward fix to me.

LGTM

LGTM

Hi Paul and Joel, thanks for your help here, appreciate it! I thought this was
a nice little addition, low-hanging fruit, but it looks like it turns into
proper project! :-) Also from our discussions, it looks like we have not just
one but a few problems. If I try to summarise them:

Looks very reasonable to me.

AMDGPU does this a lot intentionally for a shared label prefix and unique per run like ones for different subtargets

Hi Joel, thanks for taking another look!

Friendly ping.

Looks like a straightforward fix/addition to me.

LGTM

Cheers, LGTM!

Had a first look, mostly nits.

Apologies for being late to the party, but I am now looking into this too because we've seen some significant regressions with this change committed.
I am not blaming this commit, not yet, because I haven't fully understood the problem yet. As I am new to this area, it wanted to dump some initial thoughts here (because it takes me some time to get up to speed), perhaps people can comment.

• Instead of a Dk_Note, it is a DK_Warning now.
• Added test cases; hopefully demonstrating that when we have a prefix list, and one of them is used, we don't issue the warning.