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

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

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

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

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

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

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

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

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

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

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

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

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

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 ___

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

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

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

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

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

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

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

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

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

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

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

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?

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

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

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

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

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

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

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

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

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

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 =

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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/

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

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

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

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

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

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