Can you please rebase since the patch does not seem to apply?

In D154422#4470693, @arsenm wrote:

We’ve specifically avoided adding intrinsics for easy to match instructions. This needs a semantic justification over just emitting the expanded bit sequence. It’s a huge amount of work teaching every part of the compiler the equivalent bit optimizations.

Thanks! Closing this revision.

In D151691#4380433, @nikic wrote:

In D151691#4380422, @tsymalla wrote:

In D151691#4380411, @nikic wrote:

Branch on undef is undefined behavior, see https://llvm.org/docs/LangRef.html#id33:

If ‘cond’ is poison or undef, this instruction has undefined behavior.

Either your source program has undefined behavior, or some optimization incorrectly introduced the branch on undef. If the latter, you may want to look through the print-after-all trace to identify when the br undef first appears. It's possible that some optimization is failing to freeze the condition.

I know branching and switching on undefs is undefined behavior, and I know that the undef branch is intended. In no way the whole loop as marked in the pseudo-code should be optimized away.

I don't understand, why should the loop not be optimized away? Can you maybe provide a failing alive2 proof for the case you have in mind?

In D151691#4380411, @nikic wrote:

Branch on undef is undefined behavior, see https://llvm.org/docs/LangRef.html#id33:

If ‘cond’ is poison or undef, this instruction has undefined behavior.

Either your source program has undefined behavior, or some optimization incorrectly introduced the branch on undef. If the latter, you may want to look through the print-after-all trace to identify when the br undef first appears. It's possible that some optimization is failing to freeze the condition.

Simplified code

Regarding the lit test from andorbitset.ll, I don't think there's a simple way to preserve the bitset instruction with this change.

Fix nit
Change test order a bit to prevent BB renaming change

Implement shouldSinkOperands, remove old implementation from AMDGPULateCodegenPrepare.

In D150347#4341666, @tsymalla wrote:

In D150347#4341606, @arsenm wrote:

Generic CGP already has tryToSinkFreeOperands, don't know why this wouldn't already work for fabs/fneg

When I tested the implementation in AMD CGP, it did not work because the CSE pass did eliminate some of them again. I will double-check if I can make tryToSinkFreeOperands work similarly.

In D150347#4341606, @arsenm wrote:

Generic CGP already has tryToSinkFreeOperands, don't know why this wouldn't already work for fabs/fneg

Simplify checks a bit.

What is the current state of this?

LGTM

Can you please add some MIR test cases in addition to your LLVM IR tests? You already put some example code in the discussion, but I believe a test that shows the transformation on a GMIR level could be helpful.

Can you re-apply clang-format on AMDGPULegalizerInfo.cpp please?

A few nits / questions, otherwise LGTM.

LGTM

clang-format

It seems like this is breaking the LegalizerHelper.h / the LegalizerHelperTest:
/var/lib/buildkite-agent/builds/llvm-project/llvm/include/llvm/CodeGen/GlobalISel/LegalizerHelper.h:47:7: note: candidate constructor (the implicit move constructor) not viable: requires 1 argument, but 4 were provided
/var/lib/buildkite-agent/builds/llvm-project/llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp:641:19: error: no matching constructor for initialization of 'llvm::LegalizerHelper'

LegalizerHelper Helper(*MF, Info, Observer, B);

Somethign went wrong during applying the patch. Could you please have a look?
https://buildkite.com/llvm-project/diff-checks/builds/142783#01851b23-3226-4e98-bac5-219865bb07b5

Ping.
If this is getting too complicated, we could still think about moving to intrinsics.

Remove linter message

Added test to show the isDivergence behavior (copied from another test).
Changed run line to not test a specific architecture. This adds a few
s_movs to the lit tests, but shows the behavior of the isDivergence test
(the GFX10 behavior would be different).

Made comments more concise.

Moving constant check to visitExtractElementInst

Addressed review comments.

Don't access unreachable path when trying to constant-fold an extractelement with a select as index operand.

In D137934#3936962, @spatel wrote:

In D137934#3928765, @tsymalla wrote:

@spatel Thanks for adding these tests. These work in general, except the case where the second operand of the ExtractElement instruction is not a constant. The reason is that currently constantFoldOperationIntoSelectOperand expects both operands to be either the original select or a constant, but not an additional select, so a new extractelement instruction can be created to extract the actual value. If one of the operands is another select, it could be obvious that the sequence shows OOB behavior:

%e = select i1 %c, i32 3, i32 4
%s = select i1 %c, <3 x i32> <i32 2, i32 3, i32 4>, <3 x i32> <i32 5, i32 6, i32 7>
%r = extractelement <3 x i32> %s, i32 %e
ret i32 %r

Even if I know how to make the regular case work, I'll wrap my head around that particular issue.

Just bail out if the extract index value is not an immediate constant? The case where the index value is a select-of-constants that can be reduced might be another patch, but that doesn't seem like a common pattern.

@spatel Thanks for adding these tests. These work in general, except the case where the second operand of the ExtractElement instruction is not a constant. The reason is that currently constantFoldOperationIntoSelectOperand expects both operands to be either the original select or a constant, but not an additional select, so a new extractelement instruction can be created to extract the actual value. If one of the operands is another select, it could be obvious that the sequence shows OOB behavior:

Fix assertion failures

Removed the whole custom implementation as InstCombine is already able
to do the constant folding, but the Visitor for extractelement needs an
additional check to prevent folding the constants into select operands
when the select uses a condition vector.

In D137934#3926911, @foad wrote:

You don't need your new function FoldExtractElementSelectConstVector at all. All you should need to do is to call FoldOpIntoSelect from visitExtractElementInst, and add (trivial) support for ExtractElementInst in foldOperationIntoSelectOperand. For example: https://reviews.llvm.org/differential/diff/475368/

Remove superfluous dyn_cast for getAggregateElement.

Remove OOB logic. Simple cases will be handled earlier, so the relevant code
path will not be visited.

Update lit tests.

Add additional test to show folding for other types.
Use Constants instead of ConstantInt.
Conditions must be Integer types, no vector types.

In D137934#3925091, @spatel wrote:

This is missing at least one critical pattern-matching constraint (and regression test - because this will crash):

define i32 @extelt_vecselect_const_operand_vector(<3 x i1> %c) {
  %s = select <3 x i1> %c, <3 x i32> <i32 2, i32 3, i32 4>, <3 x i32> <i32 5, i32 6, i32 7>
  %r = extractelement <3 x i32> %s, i32 2
  ret i32 %r
}

• Move logic to FoldOpIntoSelect.
• Remove superfluous use check for the extractelt instruction.
• Make use of early returns.
• Add more / fix existing comments.

In D137934#3924774, @foad wrote:

extractelt (select %cond, <vec1>, <vec2>), %c1, %c2 ->
select %cond, <vec1>[c1], <vec2>[c2]

What are %c1 and %c2? Surely these is only one index?

Also your description does not make it clear that you are only doing this for constant vectors. For general vectors it could be: extractelt (select %cond, <vec1>, <vec2>), %c -> select %cond, (extractelt <vec1>, %c), (extractelt <vec2>, %c)

In D137934#3924785, @nikic wrote:

Can we extend FoldOpIntoSelect to handle this instead?

Fixed error on Windows build and whitespace issues.

This change handles most of the tree structures, but not
all of them, because this would add way more complexity.