- User Since
- Jan 26 2016, 2:17 AM (150 w, 2 d)
Mon, Dec 3
Fri, Nov 30
Thanks for taking a look!
Thu, Nov 29
Tue, Nov 27
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?
Mon, Nov 26
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:
- we want to error for overlapping dag checks,
- we want to error for the example in this diff: 1 but not all prefixes are checked,
- 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)
- CHECK-LABEL is confusing and probably mostly misunderstood
Sounds like that could be an error, rather than a warning, perhaps?
Fri, Nov 23
- 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
Thu, Nov 22
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:
Wed, Nov 21
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!
Tue, Nov 20
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.
Mon, Nov 19
Thanks, looks okay to me.
Fri, Nov 16
A little bit of progress:
Wed, Nov 14
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:
Tue, Nov 13
Sorry, just a quick message: I'm a few days out of office, and will get back to this end of this week.
Nov 9 2018
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.
Nov 8 2018
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:
Nov 7 2018
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
Nov 6 2018
Hi Joel, thanks for taking another look!
Nov 5 2018
Looks like a straightforward fix/addition to me.
Nov 1 2018
Had a first look, mostly nits.
Oct 31 2018
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.
Oct 29 2018
- 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.
Oct 26 2018
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:
Oct 25 2018
I probably need to add a regression test for this, but wanted to check this idea with you first.
Looks OK to me. Thanks
Oct 17 2018
Oct 16 2018
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.
Oct 12 2018
Looks fine to me
Oct 11 2018
Oct 4 2018
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.
Oct 2 2018
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.
Sep 26 2018
Thanks for clarifying! Looks good, with only a nit.