clang-format

updated condbinops.ll

In D155344#4512821, @craig.topper wrote:

Where did the tests go?

Addressed comments
Updated failing tests

@asb

Did you have thoughts on those?

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.

In D155328#4502297, @craig.topper wrote:

In D155328#4502274, @mgudim wrote:

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?

There isn’t a generic combine for this on select after type legalization. It depends on getBooleanContents to know the value for true. So I think it only exists for i1.

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.

addressed comments:
(1) Moved definitions of ConstSelOp and ConstSelOpNode lower.
(2) Added check DAG.isSafeToSpeculativelyExecute(...) to check if commuting select and binop is legal.

@craig.topper is it ok to recommit this?

@abs Yes, will try to commit this today.

Addressed comments: use plain array instead of SmallVector

Changed ArrayRef to SmallVector in two more lines, which I missed in the previous update.

(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.

@djtodoro Thanks for working on this. LGTM, given that you fix the tests. I would feel more comfortable if someone else (@asb, @craig.topper, @reames) gave a final approval.

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.

updated tests

This also works for CTLZ.

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.

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.

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.

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?

Thanks @craig.topper , I'll merge it (once I figure out the process). What about https://reviews.llvm.org/D151750 Can it be merged after this one?

Pre-merge checks passed.

Addressed comments.
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.

I have posted a patch for folding binop into select: https://reviews.llvm.org/D152147
That solves the degradation that @craig.topper pointed out.

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

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

@craig.topper

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?

Posting this as a draft to get some feedback / advice. I am not sure about the following points:

Build passed.

@fhahn @nikic Can someone please merge this patch for me? I don't have commit rights yet.

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.

@nikic Thanks for taking a look!

@reames Friendly ping.

Removed sve-select.s which was added by accident.

Added necessary checks missing in previous version. This fixed the failing tests.

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.

@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?

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.

In D137422#3923194, @craig.topper wrote:

Do the SETLE and SETGE cases occur? I thought DAGCombine always changes those to SETLT and SETGE by changing the constant?

In D72382#1824854, @jdoerfert wrote:

This should fix PR887: https://bugs.llvm.org/show_bug.cgi?id=887
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.

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.