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.

This time with the correct diff attached; the previous one had the test cases swapped.

Ah, I finally got it, and many thanks for explaining! I know see too that the mnemonic is totally irrelevant here (I got hugely distracted by vadd.f32).
I've learned something today! :-)

However, it seems wrong for the compiler to assign s0 as an operand without having FP registers available.

Thanks for looking at this! Answering this one first:

I probably need to add a regression test for this, but wanted to check this idea with you first.

Looks OK to me. Thanks

cheers, LGTM

With one bonus question, are the fused operations fast on the M7..?

Oops, sorry, forgot to reply, and also got distracted by a few other things. I ran one bigger benchmark, and didn't see anything worth mentioning. A first preliminary conclusion: looks like we don't miss much by not doing this lowering here. Disclaimer: I've tested only on Arm targets, definitely not on all interesting architecture combinations, and a handful of benchmarks.

Would it be worth adding a test for the M7 though? We seem to be a little lacking in our m-class FP tests.

should the f64 version not also be tested?

I think moving VFP4 check into the useFPVMLx method would help make this easier to read.

Looks fine to me

I am doing some experiments with a hack that simply comments out the call to InstCombiner::SimplifyAnyMemTransfer. I've ran 3 smaller benchmarks. 2 didn't show any difference. The 3rd shows 1 tiny regression and 1 tiny improvement in a test case, and will probably not even show a difference in the geomean. I am doing this as a background task; tomorrow I will ran bigger benchmarks, on different platforms. But I guess we need some numbers for non-Arm platforms too.

Thanks!

The ultimate goal would be to simply always canonicalize to memcpy and not expand it ever in instcombine as mentioned in D52081.

@efriedma : apologies for the ping, but does this look reasonable?

Looks okay to me.

Thanks for clarifying! Looks good, with only a nit.

LGTM

LGTM