- User Since
- Jan 26 2016, 2:17 AM (160 w, 4 d)
Thu, Feb 21
Tue, Feb 19
The ACLE has been updated and a new version with change included will be released soon.
Sun, Feb 17
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.
Fri, Feb 15
Thu, Feb 14
Wed, Feb 13
LGTM, with one nit inline.
Perhaps wait a day in case someone else has comments.
Mon, Feb 11
Thanks Eli, also for your thoughts on this:
Fri, Feb 8
Looks okay to me, with one nit inline.
Thu, Jan 31
Wed, Jan 30
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 .
Tue, Jan 29
Mon, Jan 28
Fri, Jan 25
Jan 23 2019
Jan 22 2019
Was just getting up to speed with this again; just a first round of nits before I start looking at the actual functional change.
Jan 21 2019
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:
Jan 14 2019
The AArch64 side LGTM. Also adding Oliver and Sjoerd as they worked on FP16 side as well, in case they have any thoughts.
Jan 9 2019
Jan 8 2019
Thanks, looks okay to me. Perhaps wait a day with committing to give people one more chance to comment.
Jan 7 2019
Now that we have MIR tests, do we still need the added .ll test?
Dec 3 2018
Nov 30 2018
Thanks for taking a look!
Nov 29 2018
Nov 27 2018
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?
Nov 26 2018
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?
Nov 23 2018
- 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
Nov 22 2018
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:
Nov 21 2018
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!
Nov 20 2018
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.
Nov 19 2018
Thanks, looks okay to me.
Nov 16 2018
A little bit of progress:
Nov 14 2018
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:
Nov 13 2018
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.