- User Since
- Jan 15 2015, 3:04 AM (196 w, 2 d)
Mar 1 2018
Buildbot failing here: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap-msan/builds/2998
Feb 28 2018
Feb 23 2018
If nobody does it before, I'll give it a try early next week.
Feb 15 2018
I'll commit this fix now to prevent other buildbots to fail.
Related fix for a silly errata in one of the tests that is breaking some Windows buildbots:
Committed now. @rengolin many thanks for the review!
What about 32-bit integers?
Feb 13 2018
There is still the possibility that someone tries to use 't' for a vector of two doubles. Only single-precision is allowed in vector operations for 32-bit architectures, so doing something like this would be illegal:
Added tests for int vectors. Allowing integers to go to FP/vector registers is
useful because FP/int conversion instructions (i.e. VCVT) need that.
Feb 12 2018
AFAICS, the current approach just checks the size of the type, not the size of the sub-type. f64 or even integer types could still leak in, no?
To prove they're not, we need tests making sure they break if you try.
Feb 7 2018
Feb 6 2018
Feb 1 2018
Sorry, I missed the sanitizer failure in yesterday's buildbot message and thought it wasn't related to this patch. I will leave some time for @thebolt to fix it, otherwise maybe I can take a look.
Jan 31 2018
Committed now. Thanks for the patch @thebolt!
I'll do it.
Jan 29 2018
LGTM now. In other circumstances, I would wait for someone more experienced than me, but this is a small peephole optimization. I've also tested the patch and it works correctly, so I think it has very little risk.
Jan 26 2018
The logic & testing LGTM. Just a couple of coding standard nits.
Jan 23 2018
Waiting a bit just in case there's extra feedback for the last change. I will commit this patch later today.
Jan 22 2018
VT should be a vector in addTypeForNEON, so the check can be an assert
instead of a proper test in the conditional.
Thanks for the suggestion, very helpful :)
Instead of an ugly switch, reuse already-existing functionality in LLVM to do
the conversion of FP vector types into int vector types.
Jan 18 2018
Jan 17 2018
Fixed big-endian bitconvert patterns and extensive testing for half-float vectors.
Jan 16 2018
I have fixed the bug as olista01 suggested, which is more straightforward than
my previous fix.
Jan 15 2018
Hi Sjoerd, thanks for the review. I have attached some thoughts on your comments and I will upload a new patch soon.
Jan 9 2018
Dec 4 2017
Dec 1 2017
I have also updated the existing tests and added a couple more.
Use !isa<GlobalAddressSDNode> to check if the call is through a function pointer
Nov 17 2016
Thanks all for the reviews!
Cleaned up tests as per Renato's comments.
Nov 16 2016
More testing: add tests for AAPCS and for floating-point arguments to variadic/non-variadic functions.
Could you have a look at this patch? I see no reason to restrict the tail call optimizations to variadic functions: the AAPCS restrictions seem to be already checked for non-variadic functions. The LLVM regression tests passed, and I have also tested with a few benchmarks and they all seem to work fine.
Nov 11 2016
Traverse use list instead of user list to naturally avoid duplicates. Commit message modified accordingly.
Sorry, forget what I said. I didn't get what you were trying to do at first (I've never used the "uses"). I will do the change, as this makes more clear that the operand we are interested in is the condition (i.e. operand 0).
Nov 10 2016
I have investigated the -preserve-ll-uselistorder option. It sets option "ShouldPreserveUseListOrder" to true in the module printer. According to the documentation:
Nov 9 2016
Could you have a look at this fix as per your comments in D26391?
Thanks for the comment @efriedma. I committed before I saw it, but I agree with you that this change will affect determinism of the use-lists when two of the selects on the list depend on each other. I will create a separate revision for the change and add you as reviewer.
Nov 8 2016
@sebpop , thanks for the quick response!
Could you review this patch to JumpThreading? This is an update to D25477, which was reverted two weeks ago as it was breaking LNT and some bootstrap buildbots.
Oct 21 2016
Thanks all for the reviews. Could you please review the requested formatting changes and approve if appropriate?
Oct 17 2016
James, thanks for the review and also for adding the new reviewers. I will change the lower/upper case problem with the next round of comments, when I get feedback from others.
Oct 11 2016
Sep 13 2016
Renato, James, thank you very much for the review.
Sep 12 2016
Removed vector float intrinsic test from this review. That
part of the patch has now been committed and the review can
be followed in https://reviews.llvm.org/D24398
Sep 9 2016
On second thoughts, I have tested with a modern gcc and it marks all functions with either .thumb or .arm (equivalent to .code 16/32). So, essentially, that behaviour is what this patch suggests. Another thing that surprised me is that Thumb appears to be the default, contrary to the LLVM default which is ARM.
Sep 8 2016
The changes to the vfloatintrinsics tests are independent from the functional change to add ".code 32", although I discovered them while working on this bug. I will close this review, and create a separate one with the test changes only.
Ah, sorry. They are two different changes in my tree, and I thought arcanist would preserve that but it didn't. I will make sure that they are submitted as separate patches.
Sep 7 2016
Additional checks added to the new tests.
Right, it makes sense. I didn't know that these sort of optimizations were also undertaken in the selection DAG.
Sep 6 2016
Sep 1 2016
Aug 25 2016
Sorry and thanks!
Aug 24 2016
Hello Renato, Duncan, James,
Jun 23 2016
Using std::max() now. Near that, I also made an implicit cast explicit, just for readability.
Jun 22 2016
Second round of changes addressed. Thanks!
Jun 21 2016
Same fix for another test.
Small fix for one of the tests
Addressed concerns about refactoring, comments and testing
Jun 20 2016
Hello mcrosier, jmolloy, rengolin, would you have a few minutes to review this patch?
Jun 15 2016
Oh ok, I was told that adding llvm-commits after the revision is created doesn't send the e-mail to the list. I will try that next time :)
Abandoning this review as I forgot to add the LLVM list.
Mar 15 2016
Mar 10 2016
Hello, could you please review these tests for Cortex-R8?
Jan 28 2015
Thank you James,
Jan 21 2015
Hello Yin, thank you very much for looking at the code.
Jan 15 2015
James, thank you for the review,