- User Since
- Oct 3 2019, 10:50 AM (208 w, 5 d)
Aug 1 2023
Jul 31 2023
Jul 28 2023
Jul 25 2023
Updated failing tests
Jul 18 2023
Did you have thoughts on those?
Jul 17 2023
removed FADD and FSUB
removed the test, it will be commited in a separate patch
added 32-bit versions
removed fadd and fsub because 0.0f is not an identity for these operations.
Jul 14 2023
LGTM, but it seems weird to me that we have to re-implement such basic optimisations instead of generic code doing this. @craig.topper @asb I know I asked this is the call, but I still don't understand why we emit target-specific node for SELECT so early?
I have accidentally commited this in "Needs Review" state. What should I do?
rebased on current main.
(1) Moved definitions of ConstSelOp and ConstSelOpNode lower.
(2) Added check DAG.isSafeToSpeculativelyExecute(...) to check if commuting select and binop is legal.
Jul 13 2023
@craig.topper is it ok to recommit this?
Jul 12 2023
@abs Yes, will try to commit this today.
Jul 11 2023
Addressed comments: use plain array instead of SmallVector
Changed ArrayRef to SmallVector in two more lines, which I missed in the previous update.
Jul 10 2023
(1) Added a check if FoldConstantArithmetic returns null.
(2) Fixed the incorrect use of ArrayRef. This was causing a bug, which is not visible when compiled in Debug mode.
Jun 26 2023
Jun 22 2023
Can't reproduce the failed tests locally. Put a print statement in the code to make sure it is the most recent patch being tested.
Jun 20 2023
This also works for CTLZ.
Jun 15 2023
I'll update soon. I just wanted to first see if there are any principal objections.
Not sure why we have this restriction. I've ran most of spec without it, all the tests passed and even got some small improvements.
Uploaded diff with -u99999 for more context.
Jun 14 2023
added logic for the correct order of operands when creating new opepands of select.
added test for all four possible positions of constants in select and binop.
Jun 13 2023
I'm away from my computer, I'll revert it as soon as I'll get home today, unless someone wants to do it for me earlier.
Jun 12 2023
Jun 8 2023
see DAGCombiner::SimplifySelectCC for the logic of determining which operand is what. That code also handles CTLZ and case of reversed operands.
Does the issue exist on other targets?
Jun 7 2023
Pre-merge checks passed.
Jun 6 2023
Fixed a bug: the constants were passed to DAG.FoldConstantArithmetic in the wrong order.
Moved call to foldBinOpIntoSelectIfProfitable after combineSelectToBinOp - this way we will not fold binop if the constant of select is already 0 or -1.
Minor typo fixes.
Jun 5 2023
Jun 2 2023
Is there a way to trigger all the tests before merging?
there is this failure on windows:
Added a check if type is struct (followed visitSelect as an example)
added test freeze_struct
May 31 2023
I can see two alternatives to the problem you've pointed out.
(1) Added check for legality of AND
(2) Made the check before insertion of AND stronger. Previous check could be not enough if the type size was not a power of 2.
(3) Updated affected tests
What if the target doesn't natively support CTLZ/CTTZ and only has CTLZ_ZERO_UNDEF/CTTZ_ZERO_UNDEF will end up with a select followed by the AND? What if the target doesn't support CTTZ/CTLZ at all?
Can you please share which Name <email> to use for attribution on the commit?
May 30 2023
Posting this as a draft to get some feedback / advice. I am not sure about the following points:
Merged nested ifs into one.
@nikic Thanks for explaining, I understand now. The check was necessary for the test case you suggested. I've updated the diff and included your test case.
Added the check for potentially poison values.
May 29 2023
@nikic Thanks for taking a look!
May 18 2023
@reames Friendly ping.
Dec 15 2022
Removed sve-select.s which was added by accident.
Added necessary checks missing in previous version. This fixed the failing tests.
Dec 14 2022
Implemented approach suggested by @reames : instead of considering all the possible cases, assume that the zero constant always appears as the "false" option of select and if not, invert the comparison, switch the true and false option and call foldSelectCCToShiftAnd recursively.
Dec 8 2022
@craig.topper For the test, do you want to go all the way to assembly or stopping after isel is OK? Since the optimization is target-independent, where should the test go? Do you have further comments?
Nov 17 2022
Nov 14 2022
When I previously said that now optimization works in all the cases I was wrong. You're right that we don't need to consider the non-strict inequalities. But we still need to extend the logic to work on the "inverted" patterns where the zero is the true value in the select instruction.
Nov 13 2022
Nov 10 2022
Nov 7 2022
Nov 4 2022
Jan 20 2020
My patch does not make any difference. It looks like 887 is related to pass ordering: the load can be hoisted up into the entry block, but hoisting only runs after argpromotion
This should also fix PR42039: https://bugs.llvm.org/show_bug.cgi?id=42039
Please verify that and include the test case from there. We should mention it in the commit message and close the bug if it works.
With my patch argpromotion does not happen, so I'll add this to the test cases and mention it in commit message.