Patch updated:
Use replaceUndefsWith() directly with 0/-1 constants, so we're not implicitly assuming the behavior of the APIs.

Thinking about this again...assuming that we know what getSafeVectorConstantForBinop() is going to return is dirty. Let's just replace undefs with the values we want.

Patch updated:
Restore description of constant mask elements as integer or undef. That seemed redundant to me before, but it's better to state that explicitly, so we don't let weird stuff like constant expressions into the picture.

Patch updated:
Add code comment to better explain the transform and PPC limitation.

In D74484#1967040, @aymanmus wrote:

In D74484#1966744, @spatel wrote:

In D74484#1963795, @aymanmus wrote:

@spatel: Basically the test cases with the real motivation are the first 4.

Do you mean the first 4 with diffs, or the first 4 that are being added as new tests?

The first 4 with diff.

Where most of the other cases are there to check various "edge" cases of the added code behavior.
I think, that even if some of the tests are not in a canonical form (and can be optimized by instcombine), we still have an added value having them here in order to check the behavior of this specific pass with similar cases.
Don't you agree?

Yes, I agree that we want to have tests for edge cases to make sure that the logic is correct here. But we also should have tests that show why this patch is necessary - functions that could not be solved in regular instcombine easily.

I agree with you 100%. That's why we have several test cases.

Running the same test with -instcombine instead of -aggressive-instcombine, out of the 23 test cases in the file, the following get optimized:

• cmp_select_zext_i8_noTransformation
• cmp_select_zext_sext_i8_noTransformation
• cmp_select_signed_const_i16Const_noTransformation
• cmp_select_unsigned_const_i16Const
• cmp_select_unsigned_const_i16Const_noTransformation
• cmp_select_unsigned_const_i16_MSB1
• cmp_select_bigConst_cmp
• cmp_zext_and_minus1_noTransformation
  
  Out of these cases, the ones with "_noTransformation" suffix are there to make sure our pass does not apply any transformations, and the others include some special immediate values.
  
  Actually, I feel that I might have misunderstood your comments intention. Can you explain to me what are you suggesting that we do?

In D74484#1963795, @aymanmus wrote:

@spatel: Basically the test cases with the real motivation are the first 4.

Is this NFC now? I added some sanity tests for x86 here:
rGa2bb19c
...but this patch doesn't apply to master cleanly, so I could not verify if that wiggles or not.

Adding potential reviewers for alias analysis.

In D75362#1961474, @lebedev.ri wrote:

This looks good to me.
@spatel?

In rGf2fbdf76d8d07f6a0fbd97825cbc533660d64a37#913654, @dmgreen wrote:

Thanks, much appreciated. And thanks for adding the extra test.

Note that we don't see test diffs for simpler cases because we already handle this related pattern:

// C - ~X == X + (1+C)
if (match(Op1, m_Not(m_Value(X))))
  return BinaryOperator::CreateAdd(X, AddOne(C));

Patch updated:
Rearranged text a bit more and removed reference to the 'UndefMaskElem' code constant.

In D40633#1958062, @RKSimon wrote:

@spatel Maybe ensure we have all the test coverage from the tests that Konstantin added here?

Patch updated:
Fixed types to deal with overflow (hopefully properly this time!).

! In D76638#1959075, @steven.zhang wrote:
Yeah, it is sensitive to how we combine the nodes. I tried your patch(add FENG check for FADD), and see two extra instructions generated as you mentioned, on X86. So, we need some positive tests for this patch if it helps the codegen. What do you think ?

Abandoning - D77319 will solve the larger problem.

In D76638#1957541, @steven.zhang wrote:

In D76638#1956952, @spatel wrote:

Ping. I think this change is useful regardless of whether/how we make a larger fix for getNegatibleCost+getNegatedExpression.

I have posted a patch(https://reviews.llvm.org/D77319) to remove the getNegatibleCost. It is almost ready now. I am not sure if it is the best way , and welcome for the comments in advance. Regarding to the opportunities for this patch, can you double confirm it together with my patch to see if there is any improvement ?

Patch updated:
Widened type for scaled index and added assert.

Ping.

Patch updated - no logic changes, but rebased/reduced with new shuffle instruction.

Patch updated - no logic diffs from before but:

1. Rebased/simplified with new mask-operand-less version of shuffle (D72467, D77183).
2. Added function comment to explain the transform/motivation.

In D76983#1956732, @jeroen.dobbelaere wrote:

In D76983#1955660, @spatel wrote:

I'll draft a patch.

thanks !

In rGf2fbdf76d8d07f6a0fbd97825cbc533660d64a37#913424, @dmgreen wrote:

Yeah, the multiple uses are a pain.

How married are you to this commit? It was a fairly chunky regression, and if you don't mind reverting until we have a better fix for that code it would help get one thing off my plate.

Ping. I think this change is useful regardless of whether/how we make a larger fix for getNegatibleCost+getNegatedExpression.

In D76983#1955540, @lebedev.ri wrote:

Ie, is there consensus that forming a size-changing vector cast from a shuffle is canonical?

I would have guessed it is, yes.

@dmgreen wrote

@spatel wrote

! In rGf2fbdf76d8d07f6a0fbd97825cbc533660d64a37#913209, @dmgreen wrote:
Hello. Unfortunately we have some tests where it looks like this is needed. This code: https://godbolt.org/z/DpJuFm is no longer reaching the same minimum after this patch. Any ideas?

Whilst playing around I noticed that using "char k = (c < m) ? (c < y ? c : y) : (m < y ? m : y)", instead of the if, always ends up with worse code. Perhaps a missing fold of some sort?

I thought we had all variants of that min/max-of-nots beaten into submission, but no...
Taking a look now. Maybe we can enhance "freelyNegateValue()" to see through this...

Thanks. If I remember correctly, this example was really good at getting stuck in local minimum because of all the multiple uses going on.
Perhaps the same thing is happening again now?

In D76928#1955125, @spatel wrote:

4. Improve demanded elements analysis of x86 min/max/cmp - the x86 part of this patch, but with different tests to show the win with different operands.

These tests show a set of missed optimizations, so I recommend taking a step back and separate this into a few patches:

1. Fold x86 min/max intrinsics better - if operands are identical, the min/max simplifies away.
2. Fold x86 cmp intrinsics better (thought we had a bug report for this, but I don't see it now) - if operands are identical, the compare can simplify away (see SimplifyFCmpInst()) or change predicate.
3. Improve demanded elements analysis with isOnlyUserOf() - use generic opcode like 'mul' to show that improvement (independent of x86).
4. Improve demanded elements analysis of x86 min/max/cmp - the x86 part of this patch, but with different tests to show the win with different operands.

In D76983#1954751, @lebedev.ri wrote:

In D76983#1954722, @jeroen.dobbelaere wrote:

This patch triggers a regression on our side:

<...>

The tests expects to see:

define dso_local <4 x i16> @truncate_v_v(<4 x i32> %lhs) local_unnamed_addr #0 {
entry:
  %0 = trunc <4 x i32> %lhs to <4 x i16>
  ret <4 x i16> %0
}

which, in machine instructions, is mapped onto a vector trunc instruction.

But now, we see:

define dso_local <4 x i16> @truncate_v_v(<4 x i32> %lhs) local_unnamed_addr #0 {
entry:
  %0 = bitcast <4 x i32> %lhs to <8 x i16>
  %vecinit9 = shufflevector <8 x i16> %0, <8 x i16> undef, <4 x i32> <i32 0, i32 2, i32 4, i32 6>
  ret <4 x i16> %vecinit9
}

which is expanded into a large sequence of code going through the stack.

This looks like a simple missed transform to me, not a miscompile

In D74484#1951878, @aymanmus wrote:

Just checked the patch @spatel mentioned.
Doesn't really help with the cases provided here.

LGTM

LGTM

D72467 is not actually a parent, but adding that here because this would conflict (force another rebase of that patch).

D72467 is not actually a parent, but adding that here because this would conflict (force another rebase of that patch).

LGTM - we're trying to reduce and fix the cost model mess, so I'm fine with this step. If we want to add non-intrinsic call cost modeling in the future, it can be done without much effort when necessary.

In D76983#1951210, @dsprenkels wrote:

• Updated ExtractCast.ll test.
  
  Should I explicitly add the data layout in ExtractCast.ll, because the added assertions are only valid on little endian platforms?

I have not looked at the code changes, but I noticed the first few tests are all min/max patterns. Can you check your motivating apps/benchmarks to see if rGf2fbdf76d8d0 helps?

In D76983#1950343, @dsprenkels wrote:

Let me look at the failed test in a couple of hours.

Btw. I have commit permissions now, so I will be able to commit this myself now. :)

LGTM

It would be great if we can simplify this chunk, but I'm not sure of the history/plans. Adding @SjoerdMeijer based on the recent patch D59014.

I think we're pretty close now.
Added another test, so please rebase/update:
rGbc60cdcc3f8

In D76983#1948849, @dsprenkels wrote:

My git lingo might be off here. Are you saying you don't have commit permissions for LLVM yet?

Yup. Should I ask for it? (I don't know what the etiquette is; i.e. how many patches before one should request commit access.)

LGTM - I'll push this.