[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Craig Topper via Phabricator via cfe-commits]

craig.topper abandoned this revision.
craig.topper added a comment.

This got replaced by D85765 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

In D83360#2210630 , @nikic wrote:

> D85684  has landed, so we can try reapplying 
> this change.

I've put up D85765  with this patch and the 
follow ups re-enabled.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

D85684  has landed, so we can try reapplying 
this change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Juneyoung Lee via Phabricator via cfe-commits]

aqjune added a comment.

Made a patch at https://reviews.llvm.org/D84250 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Juneyoung Lee via Phabricator via cfe-commits]

aqjune added a comment.

In D83360#2162898 , @craig.topper 
wrote:

> @aqjune did you put a patch for InstSimplify doing distribution over undef 
> yet?


Sorry, making InstSimplify to safely distribute undef was a nontrivial job - 
other than updating InstSimplify to track uses, it needed rewinding folded 
undef records if simplification failed & update constant folding to track uses 
too. The folded value could be symbolic, making things more complex.
I'm trying an alternative solution for this problem, I will leave a link here 
after submission.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

@aqjune did you put a patch for InstSimplify doing distribution over undef yet?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

Merge request for 11.0 https://bugs.llvm.org/show_bug.cgi?id=46740


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Craig Topper via Phabricator via cfe-commits]

craig.topper reopened this revision.
craig.topper added a comment.
This revision is now accepted and ready to land.

Reverted in 00f3579aea6e3d4a4b7464c3db47294f71cef9e4 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

I'm going to revert this as Eric requested. And I'll ask to merge the revert to 
the 11 branch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Eric Christopher via Phabricator via cfe-commits]

echristo added a comment.

In D83360#2154637 , @aqjune wrote:

> In D83360#2154584 , @echristo wrote:
>
> > It's that even before the msan instrumentation the IR doesn't look correct 
> > - thus a miscompile.
>
>
> I'll share a prototype of the InstSimplify patch on Phabricator, in a day or 
> two; it would be great if the miscompilation is fixed with the patch.


Would it be reasonable to revert this in the meantime?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Juneyoung Lee via Phabricator via cfe-commits]

aqjune added a comment.

In D83360#2154584 , @echristo wrote:

> It's that even before the msan instrumentation the IR doesn't look correct - 
> thus a miscompile.


I'll share a prototype of the InstSimplify patch on Phabricator, in a day or 
two; it would be great if the miscompilation is fixed with the patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Eric Christopher via Phabricator via cfe-commits]

echristo added a comment.

It's that even before the msan instrumentation the IR doesn't look correct - 
thus a miscompile.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D83360#2154545 , @echristo wrote:

> We're starting to see miscompiles as we do more testing as well, just nothing 
> smaller at the moment.


Could you clarify, do you mean that this fix is causing (new) miscompiles?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Eric Christopher via Phabricator via cfe-commits]

echristo added a comment.

We're starting to see miscompiles as we do more testing as well, just nothing 
smaller at the moment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D83360#2154540 , @echristo wrote:

> This seems like something that we should then revert until we know that 
> instsimplify can be updated with a fix?


Possibility for a miscompile sounds much worse to me than a false-positive 
diag..


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Eric Christopher via Phabricator via cfe-commits]

echristo added a comment.

This seems like something that we should then revert until we know that 
instsimplify can be updated with a fix?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

FYI InstSimplify doing distribution of undef is a known bug: 
https://bugs.llvm.org/show_bug.cgi?id=33165


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Juneyoung Lee via Phabricator via cfe-commits]

aqjune added a comment.

(renaming variables for readability)

  %a = select i1 %s, i1 undef, i1 %t
  %b = xor i1 %s, 1
  %c = and i1 %a, %b

This series of reasoning happened from a single SimplifyAndInst call:

  c = a & (s ^ 1)
= (a & s) ^ (a & 1); ExpandBinOp
= ((select s, undef, t) & s) ^ a
= (select s, (undef & s), (t & s)) ^ a ; ThreadBinOpOverSelect
= (select s, (undef & s), false) ^ a   ; since s = (x == y), t = (x < y)
= (select s, false, false) ^ a ; choose undef to be false
= a
= select s, undef, t

In general, distributing `a` into operands of xor (second line) isn't sound 
because it increases the number of uses of `a`. We don't want to totally 
disable the simplification, however.

If InstSimplify never increases the number of uses in the end, we have an 
alternative solution: tracking to which value undef is folded.
Whenever an undef value is chosen to be a concrete value, the decision should 
be remembered, so the copied undefs won't be folded into different values.
In case of InstSimplify, we can identify individual undefs by Use, since 
InstSimplify won't do any transformation inside.
This means SimplifyXXX needs to return two things: the simplified value & the 
undef cache. Since InstSimplify isn't designed to do transformation directly, 
other optimizations like InstCombine should perform the final change.

Does this solution make sense? Then, I can prepare a patch for this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Juneyoung Lee via Phabricator via cfe-commits]

aqjune added a comment.

Seems like a bug in instsimplify:

  define i1 @f(i32 %x, i32 %y) {
%cmp9.not.1 = icmp eq i32 %x, %y
%cmp15  = icmp slt i32 %x, %y
%spec.select39 = select i1 %cmp9.not.1, i1 undef, i1 %cmp15
%spec.select40 = xor i1 %cmp9.not.1, 1
%spec.select  = and i1 %spec.select39, %spec.select40
ret i1 %spec.select
  }
  =>
  define i1 @f(i32 %x, i32 %y) {
%cmp9.not.1 = icmp eq i32 %x, %y
%cmp15 = icmp slt i32 %x, %y
%spec.select39 = select i1 %cmp9.not.1, i1 undef, i1 %cmp15
ret i1 %spec.select39
  }

https://godbolt.org/z/a8f7hT

Alive2 says it's incorrect: https://alive2.llvm.org/ce/z/-8Q4HL

Seems to be related with ValueTracking's isImpliedCondition since this 
optimizations happens only when operands of the two icmps are the same.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added subscribers: eugenis, guiand, vitalybuka.
vitalybuka added a comment.

After this patch we have false msan reports on code like this:

  bool iv_compare2(const int *op1, const int *op2) {
if (op1[1] != op2[1]) return op1[1] < op2[1];
for (int i = 1; i >= 0; i--) {
  if (op1[i] != op2[i]) return op1[i] < op2[i];
}
return false;
  }
  
  void foo() {
int a[2] = {};
int b[2] = {};
auto UNINITIALIZED= iv_compare2(a, b);
  }

Here it looks fine and the same as before the patch. It returns "and undef, 
false" which should be false.

  *** IR Dump After Simplify the CFG ***
  ; Function Attrs: norecurse nounwind readonly sanitize_memory uwtable
  define zeroext i1 @_Z11iv_compare2PKiS0_(i32* nocapture readonly %op1, i32* 
nocapture readonly %op2) local_unnamed_addr #0 !dbg !8 {
  entry:
%arrayidx = getelementptr inbounds i32, i32* %op1, i64 1, !dbg !10
%0 = load i32, i32* %arrayidx, align 4, !dbg !10
%arrayidx1 = getelementptr inbounds i32, i32* %op2, i64 1, !dbg !11
%1 = load i32, i32* %arrayidx1, align 4, !dbg !11
%cmp.not = icmp eq i32 %0, %1, !dbg !12
br i1 %cmp.not, label %for.cond, label %if.then, !dbg !10
  
  if.then:  ; preds = %entry
%cmp4 = icmp slt i32 %0, %1, !dbg !13
ret i1 %cmp4, !dbg !14
  
  for.cond: ; preds = %entry
%2 = load i32, i32* %op1, align 4, !dbg !15
%3 = load i32, i32* %op2, align 4, !dbg !16
%cmp9.not.1 = icmp eq i32 %2, %3, !dbg !17
%cmp15 = icmp slt i32 %2, %3
%spec.select39 = select i1 %cmp9.not.1, i1 undef, i1 %cmp15, !dbg !15
%spec.select40 = select i1 %cmp9.not.1, i1 false, i1 true, !dbg !15
%spec.select = and i1 %spec.select39, %spec.select40
ret i1 %spec.select
  }

However with this patch after the next transformation it breaks the code:
Now it returns undef instead of false if %2 == %3

  *** IR Dump After Combine redundant instructions ***
  ; Function Attrs: norecurse nounwind readonly sanitize_memory uwtable
  define zeroext i1 @_Z11iv_compare2PKiS0_(i32* nocapture readonly %op1, i32* 
nocapture readonly %op2) local_unnamed_addr #0 !dbg !8 {
  entry:
%arrayidx = getelementptr inbounds i32, i32* %op1, i64 1, !dbg !10
%0 = load i32, i32* %arrayidx, align 4, !dbg !10
%arrayidx1 = getelementptr inbounds i32, i32* %op2, i64 1, !dbg !11
%1 = load i32, i32* %arrayidx1, align 4, !dbg !11
%cmp.not = icmp eq i32 %0, %1, !dbg !12
br i1 %cmp.not, label %for.cond, label %if.then, !dbg !10
  
  if.then:  ; preds = %entry
%cmp4 = icmp slt i32 %0, %1, !dbg !13
ret i1 %cmp4, !dbg !14
  
  for.cond: ; preds = %entry
%2 = load i32, i32* %op1, align 4, !dbg !15
%3 = load i32, i32* %op2, align 4, !dbg !16
%cmp9.not.1 = icmp eq i32 %2, %3, !dbg !17
%cmp15 = icmp slt i32 %2, %3
%spec.select39 = select i1 %cmp9.not.1, i1 undef, i1 %cmp15, !dbg !15
ret i1 %spec.select39
  }

The msan reasonably reports a bug.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a subscriber: tgt.
nlopes added a comment.

In D83360#2142457 , @efriedma wrote:

> > that's fine but I still don't understand why the counterexample to my 
> > version says %x2 in @src can be undef
>
> If I'm understanding correctly, this reduces to something like the following:
>
> define i32 @src() {
>
>   %x2 = freeze i32 undef
>   ret i32 %x2
>
> }
>
> define i32 @tgt() {
>
>   ret i32 undef
>
> }
>
> This seems a little suspect, yes.


This is a known bug: https://github.com/AliveToolkit/alive2/issues/3
gotta fix this soon.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a subscriber: tgt.
efriedma added a comment.

> that's fine but I still don't understand why the counterexample to my version 
> says %x2 in @src can be undef

If I'm understanding correctly, this reduces to something like the following:

define i32 @src() {

  %x2 = freeze i32 undef
  ret i32 %x2

}

define i32 @tgt() {

  ret i32 undef

}

This seems a little suspect, yes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[John Regehr via Phabricator via cfe-commits]

regehr added a comment.

> Did you mean to check something like the following?
> 
>   define i32 @src(i1 %cond, i32 %x) {
> %x2 = freeze i32 %x
> %s = select i1 %cond, i32 %x2, i32 undef
> ret i32 %s
>   }
>   
>   define i32 @tgt(i1 %cond, i32 %x) {
> %x2 = freeze i32 %x
> ret i32 %x2
>   }

that's fine but I still don't understand why the counterexample to my version 
says %x2 in @src can be undef


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D83360#2140241 , @craig.topper 
wrote:

> Alive does like this https://alive2.llvm.org/ce/z/yhibbe which is what I was 
> going to implement.


right. There's a guaranteedNonPoison (or similar name) in ValueTracking that 
can be used I guess.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

Alive does like this https://alive2.llvm.org/ce/z/yhibbe which is what I was 
going to implement.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

In D83360#2140224 , @regehr wrote:

> @craig.topper ok, I agree that should work. alive doesn't like it -- is this 
> an alive bug @nlopes? a freeze should not yield undef.
>  https://alive2.llvm.org/ce/z/mWAsYv


Did you mean to check something like the following?

  define i32 @src(i1 %cond, i32 %x) {
%x2 = freeze i32 %x
%s = select i1 %cond, i32 %x2, i32 undef
ret i32 %s
  }
  
  define i32 @tgt(i1 %cond, i32 %x) {
%x2 = freeze i32 %x
ret i32 %x2
  }


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[John Regehr via Phabricator via cfe-commits]

regehr added a comment.

@craig.topper ok, I agree that should work. alive doesn't like it -- is this an 
alive bug @nlopes? a freeze should not yield undef.
https://alive2.llvm.org/ce/z/mWAsYv


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

Wasn't @majnemer asking about

  define i32 @src(i1 %cond, i32 %x) {
%xf = freeze i32 %x
%s = select i1 %cond, i32 %xf, i32 undef
ret i32 %s
  }

which is legal. I'm going to work on supporting known non-poison cases.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[John Regehr via Phabricator via cfe-commits]

regehr added a comment.

@majnemer should work: https://alive2.llvm.org/ce/z/vL4yn4


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[David Majnemer via Phabricator via cfe-commits]

majnemer added inline comments.



Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4121-4125
-  if (isa(TrueVal))   // select ?, undef, X -> X
-return FalseVal;
-  if (isa(FalseVal))   // select ?, X, undef -> X
-return TrueVal;
-

Can we still do these optimizations when `X` is a frozen value?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> Should we remove the handling from llvm::ConstantFoldSelectInstruction

It seems silly to remove the handling from InstSimplify, but not constant 
folding.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

In D83360#2139933 , @efriedma wrote:

> Please also add testcases with select constant expressions, to test constant 
> folding.


Should we remove the handling from llvm::ConstantFoldSelectInstruction


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Please also add testcases with select constant expressions, to test constant 
folding.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Craig Topper via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9b1e95329af7: [InstSimplify] Remove select ?, undef, X - 
X and select ?, X, undef - X… (authored by craig.topper).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83360/new/

https://reviews.llvm.org/D83360

Files:
  clang/test/CodeGen/arm-mve-intrinsics/dup.c
  llvm/lib/Analysis/InstructionSimplify.cpp
  llvm/test/Transforms/InstCombine/select.ll
  llvm/test/Transforms/InstSimplify/select.ll

Index: llvm/test/Transforms/InstSimplify/select.ll
===
--- llvm/test/Transforms/InstSimplify/select.ll
+++ llvm/test/Transforms/InstSimplify/select.ll
@@ -750,3 +750,43 @@
   %c3 = select i1 %c1, i1 %c2, i1 false
   ret i1 %c3
 }
+
+; Negative tests to ensure we don't remove selects with undef true/false values.
+; See https://bugs.llvm.org/show_bug.cgi?id=31633
+; https://lists.llvm.org/pipermail/llvm-dev/2016-October/106182.html
+; https://reviews.llvm.org/D83360
+define i32 @false_undef(i1 %cond, i32 %x) {
+; CHECK-LABEL: @false_undef(
+; CHECK-NEXT:[[S:%.*]] = select i1 [[COND:%.*]], i32 [[X:%.*]], i32 undef
+; CHECK-NEXT:ret i32 [[S]]
+;
+  %s = select i1 %cond, i32 %x, i32 undef
+  ret i32 %s
+}
+
+define i32 @true_undef(i1 %cond, i32 %x) {
+; CHECK-LABEL: @true_undef(
+; CHECK-NEXT:[[S:%.*]] = select i1 [[COND:%.*]], i32 undef, i32 [[X:%.*]]
+; CHECK-NEXT:ret i32 [[S]]
+;
+  %s = select i1 %cond, i32 undef, i32 %x
+  ret i32 %s
+}
+
+define <2 x i32> @false_undef_vec(i1 %cond, <2 x i32> %x) {
+; CHECK-LABEL: @false_undef_vec(
+; CHECK-NEXT:[[S:%.*]] = select i1 [[COND:%.*]], <2 x i32> [[X:%.*]], <2 x i32> undef
+; CHECK-NEXT:ret <2 x i32> [[S]]
+;
+  %s = select i1 %cond, <2 x i32> %x, <2 x i32> undef
+  ret <2 x i32> %s
+}
+
+define <2 x i32> @true_undef_vec(i1 %cond, <2 x i32> %x) {
+; CHECK-LABEL: @true_undef_vec(
+; CHECK-NEXT:[[S:%.*]] = select i1 [[COND:%.*]], <2 x i32> undef, <2 x i32> [[X:%.*]]
+; CHECK-NEXT:ret <2 x i32> [[S]]
+;
+  %s = select i1 %cond, <2 x i32> undef, <2 x i32> %x
+  ret <2 x i32> %s
+}
Index: llvm/test/Transforms/InstCombine/select.ll
===
--- llvm/test/Transforms/InstCombine/select.ll
+++ llvm/test/Transforms/InstCombine/select.ll
@@ -2273,3 +2273,43 @@
   %sel = select i1 %cond, i32 %phi, i32 %A
   ret i32 %sel
 }
+
+; Negative tests to ensure we don't remove selects with undef true/false values.
+; See https://bugs.llvm.org/show_bug.cgi?id=31633
+; https://lists.llvm.org/pipermail/llvm-dev/2016-October/106182.html
+; https://reviews.llvm.org/D83360
+define i32 @false_undef(i1 %cond, i32 %x) {
+; CHECK-LABEL: @false_undef(
+; CHECK-NEXT:[[S:%.*]] = select i1 [[COND:%.*]], i32 [[X:%.*]], i32 undef
+; CHECK-NEXT:ret i32 [[S]]
+;
+  %s = select i1 %cond, i32 %x, i32 undef
+  ret i32 %s
+}
+
+define i32 @true_undef(i1 %cond, i32 %x) {
+; CHECK-LABEL: @true_undef(
+; CHECK-NEXT:[[S:%.*]] = select i1 [[COND:%.*]], i32 undef, i32 [[X:%.*]]
+; CHECK-NEXT:ret i32 [[S]]
+;
+  %s = select i1 %cond, i32 undef, i32 %x
+  ret i32 %s
+}
+
+define <2 x i32> @false_undef_vec(i1 %cond, <2 x i32> %x) {
+; CHECK-LABEL: @false_undef_vec(
+; CHECK-NEXT:[[S:%.*]] = select i1 [[COND:%.*]], <2 x i32> [[X:%.*]], <2 x i32> undef
+; CHECK-NEXT:ret <2 x i32> [[S]]
+;
+  %s = select i1 %cond, <2 x i32> %x, <2 x i32> undef
+  ret <2 x i32> %s
+}
+
+define <2 x i32> @true_undef_vec(i1 %cond, <2 x i32> %x) {
+; CHECK-LABEL: @true_undef_vec(
+; CHECK-NEXT:[[S:%.*]] = select i1 [[COND:%.*]], <2 x i32> undef, <2 x i32> [[X:%.*]]
+; CHECK-NEXT:ret <2 x i32> [[S]]
+;
+  %s = select i1 %cond, <2 x i32> undef, <2 x i32> %x
+  ret <2 x i32> %s
+}
Index: llvm/lib/Analysis/InstructionSimplify.cpp
===
--- llvm/lib/Analysis/InstructionSimplify.cpp
+++ llvm/lib/Analysis/InstructionSimplify.cpp
@@ -4118,11 +4118,6 @@
   if (TrueVal == FalseVal)
 return TrueVal;
 
-  if (isa(TrueVal))   // select ?, undef, X -> X
-return FalseVal;
-  if (isa(FalseVal))   // select ?, X, undef -> X
-return TrueVal;
-
   // Deal with partial undef vector constants: select ?, VecC, VecC' --> VecC''
   Constant *TrueC, *FalseC;
   if (TrueVal->getType()->isVectorTy() && match(TrueVal, m_Constant(TrueC)) &&
Index: clang/test/CodeGen/arm-mve-intrinsics/dup.c
===
--- clang/test/CodeGen/arm-mve-intrinsics/dup.c
+++ clang/test/CodeGen/arm-mve-intrinsics/dup.c
@@ -242,7 +242,8 @@
 // CHECK-NEXT:[[TMP1:%.*]] = call <8 x i1> @llvm.arm.mve.pred.i2v.v8i1(i32 [[TMP0]])
 // CHECK-NEXT:[[DOTSPLATINSERT:%.*]] = insertelement <8 x half> undef, half [[A:%.*]], i32 0
 // CHECK-NEXT:[[DOTSPLAT:%.*]] = shufflevector <8 x