User Details
- User Since
- Nov 19 2012, 1:00 PM (539 w, 6 d)
Feb 3 2022
Hi Björn. Thank you for looking into this bug and reproducing the problem on aarch64. It sounds like you found the underlying issue in checkMergeStoreCandidatesForDependencies. Do you need help with anything?
Jan 20 2022
Jan 19 2022
Ah, I did miss something. I'll rewrite the second check: "Assume ((c +1 ) << y) != 127"
@craig.topper Thank you for the review, and for writing the proof, of course. This is my understanding of the code and of your proof.
Jan 18 2022
Jan 14 2022
@lebedev.ri Okay. I'll add the extra tests, but I'll do it in a separate diff because I've already landed the pre-commit diff that added the extra tests. I also need to look at the optimization that craig pointed out.
@spatel Thank you for the review. I added the pre-commit tests in a new diff (https://reviews.llvm.org/D117338). I've also made the changes to the comment as you suggested. Are you willing to approve the diff or do you want to see the comment and do another round of review?
Jan 13 2022
@craig.topper Ah. I think that the ICMP_UGT transformation is correct, but if you don't mind I prefer to land the two changes separately. I can start a second review for the ICMP_UGT change. Is that okay with you?
@craig.topper thank you for the review. I removed the two predicated that are canonicalized before this method is called (ICMP_SGE and ICMP_UGE). I also used the script update_test_checks.py to update the check lines. I also verified with alive2 that the transformation is correct for ICMP_UGT, but only when the exact flag is present (link: https://alive2.llvm.org/ce/z/qBhxAM).
@lebedev.ri Good catch. Thank you. I verified that when 'exact' is present the transformation is valid for all predicates.
Jan 10 2022
Added the test merge_store_alias that checks that aliasing loads prevent the merging of the store operations.
@hoy Thank you for the review. Yes, the code in TryToAddCandidate and CandidateMatch checks that all of the stores have the same base. I added a second test where the load aliases and prevents the store merging.
Jan 9 2022
Dec 2 2020
Nov 3 2020
Hi Dave. I have a quick question. Have you considered tuning the current cost model? What are the compile time implications of using VPlans?
Aug 12 2020
Aug 7 2020
Jul 27 2020
@craig.topper Good catch. Thank you. I've updated the diff.
Jul 26 2020
Jul 16 2020
Jul 15 2020
@wenlei You are right. SmallVector is a better fit. I updated the patch.
Jul 14 2020
@modocache Thank you for the review. I updated the permissions of the diff. Sorry, for the trouble.
Thank you. I will make the changes and try to commit the patch.
Oct 19 2018
Apr 29 2017
LGTM!
Apr 27 2017
Aug 16 2016
Aug 5 2016
Aug 3 2016
Aug 2 2016
LGTM.
Aug 1 2016
LGTM!
Jul 29 2016
Jul 4 2016
Ayal, I commented on the bug report that I don't understand why this heuristic is useful in the general case (for loops that are not this specific loop). Are you seeing any speedups on SPEC or the LLVM test suite (or other test suites?).
Jun 20 2016
Magnus, I don't have any problem with this change because it is specific to Erlang. I suggest CC-ing the original author of adjustForHiPEPrologue, and if that person has no objections commit the change. Relying on metadata is not idea, but I understand the constraint.
Jun 13 2016
LGTM.
May 10 2016
Mar 30 2016
Hal, the code looks much better. I was thinking about this patch on the way to work this morning and I was wondering if you could mark the type <4 x i32> as legal, because you _can_ load it and store it to memory. Right?
Mar 29 2016
I don't like the approach of passing in address-of-bool as parameter argument, especially since you did not document the parameter (is it IN, is it OUT, etc).
Hal, I am not sure I understand the problem. Is the problem register pressure or the fact that store <8 x i32> is more expensive than 8 times store i32?
Mar 2 2016
LGTM.
Feb 23 2016
The patch LGTM.
Feb 9 2016
Sorry for the delay in review. This patch LGTM.
Jan 28 2016
Jan 21 2016
Sounds good. Thanks Matthew.
Did you see any performance improvements due to this commit? Did you measure compile times?
Jan 12 2016
LGTM.
Dec 29 2015
LGTM.
Dec 21 2015
Elena, please fix the cost table and don't change anything else.
We don't let the instruction Sitofp <4 x i32 > %a to <4 x f64> go through the type legalizer.
Dec 20 2015
Elena, I don't understand your comment. getTypeLegalizationCost imitates the type legalizer by splitting and promoting. It actually uses the type legalizer. What do you mean it does not support zero-extend and sign-extend?
Dec 18 2015
Thanks for explaining this Elena. Have you considered handling all of the special cases by adding them to the 'TypeConversionCostTblEntry' table? Also, have you considered improving getTypeLegalizationCost?