[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

nlopes added a comment. In D87188#2476230 , @thakis wrote: > In D87188#2470401 , @lebedev.ri > wrote: > >> In D87188#2470392 , @thakis wrote: >> >>> Heads up: Breaks a test

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

thakis added a comment. In D87188#2470401 , @lebedev.ri wrote: > In D87188#2470392 , @thakis wrote: > >> Heads up: Breaks a test for us: >> https://bugs.chromium.org/p/chromium/issues/detail?id=1161542 >> >> (No

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

lebedev.ri added a comment. In D87188#2467915 , @nlopes wrote: > This patch regressed the following test of Transforms/InstCombine/abs-1.ll: > (need to drop NSW in this case). > > define i8 @nabs_canonical_3(i8 %x) { > %0: > %cmp = icmp slt i8 %x,

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

lebedev.ri added a comment. In D87188#2470392 , @thakis wrote: > Heads up: Breaks a test for us: > https://bugs.chromium.org/p/chromium/issues/detail?id=1161542 > > (No reduced repro yet, might be UB, just fyi at this point.) Thanks for headsup. For now

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

thakis added a comment. Heads up: Breaks a test for us: https://bugs.chromium.org/p/chromium/issues/detail?id=1161542 (No reduced repro yet, might be UB, just fyi at this point.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87188/new/

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

nlopes added a comment. This patch regressed the following test of Transforms/InstCombine/abs-1.ll: (need to drop NSW in this case). define i8 @nabs_canonical_3(i8 %x) { %0: %cmp = icmp slt i8 %x, 0 %neg = sub nsw i8 0, %x %abs = select i1 %cmp, i8 %x, i8 %neg ret i8 %abs }

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

mstorsjo added a comment. In D87188#2463733 , @mstorsjo wrote: > In D87188#2463566 , @lebedev.ri > wrote: > >> Once again, thank you @mstorsjo and @dmgreen! > > Awesome, thanks guys! > > I'll let you know tomorrow

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

mstorsjo added a comment. In D87188#2463566 , @lebedev.ri wrote: > Once again, thank you @mstorsjo and @dmgreen! Awesome, thanks guys! I'll let you know tomorrow in the unlikely case if this still caused some other regressions as well (or possibly

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

lebedev.ri added a comment. Once again, thank you @mstorsjo and @dmgreen! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87188/new/ https://reviews.llvm.org/D87188 ___ cfe-commits mailing list

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG897c985e1e21: [InstCombine] Canonicalize SPF to abs intrinsic (authored by lebedev.ri). Changed prior to commit:

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

lebedev.ri added a comment. In D87188#2463447 , @dmgreen wrote: > Yeah. The reproducer seems to work OK with a patch something like this: > > diff --git a/llvm/lib/Analysis/InstructionSimplify.cpp > b/llvm/lib/Analysis/InstructionSimplify.cpp > index

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

dmgreen added a comment. Yeah. The reproducer seems to work OK with a patch something like this: diff --git a/llvm/lib/Analysis/InstructionSimplify.cpp b/llvm/lib/Analysis/InstructionSimplify.cpp index 35c21a0..c517286 100644

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

lebedev.ri added a comment. In D87188#2463285 , @dmgreen wrote: > Thanks for the reproducer. I verified that it does indeed fail with this > patch. > > It seems to be doing this as a knock-on effect: https://godbolt.org/z/Y4z3je, > which does not

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

dmgreen added a comment. Thanks for the reproducer. I verified that it does indeed fail with this patch. It seems to be doing this as a knock-on effect: https://godbolt.org/z/Y4z3je, which does not verify: https://alive2.llvm.org/ce/z/PN7Rv5 ? Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

mstorsjo added a comment. In D87188#2461510 , @mstorsjo wrote: > In D87188#2461497 , @lebedev.ri > wrote: > >> @mstorsjo i've bothered @dmajor to give this a try on some of mozilla's CI, >> and unfortunately it

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

mstorsjo added a comment. In D87188#2461497 , @lebedev.ri wrote: > @mstorsjo i've bothered @dmajor to give this a try on some of mozilla's CI, > and unfortunately it came back green.. > I don't recall if i can reproduce it locally (on vanilla test suite,

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

lebedev.ri added a subscriber: dmajor. lebedev.ri added a comment. @mstorsjo i've bothered @dmajor to give this a try on some of mozilla's CI, and unfortunately it came back green.. I don't recall if i can reproduce it locally (on vanilla test suite, i'll recheck), but right now it would seem

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

mstorsjo added a comment. (Also, for studying the effect of this one, on an IR level, the difference is already present in the output of `-S -emit-llvm` from clang, unless using `-Xclang -disable-llvm-passes`, but I'm sure you already took that into account.) Repository: rG LLVM Github

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

mstorsjo added a comment. FWIW, the issue is reproducible for an aarch64-linux-gnu target as well. If it would help someone, I could make a prepackaged build for that target, with object files + this single source function that breaks it + sample input files to run it with. Repository: rG

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

mstorsjo added a comment. In D87188#2447958 , @lebedev.ri wrote: > Just to be sure, i just run the entire compilation of `vc1_block-aarch64.c` > through alive2, and as far as i can tell, it did not report any IR-level > miscompilations. > So either the

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

lebedev.ri added a subscriber: t.p.northover. lebedev.ri added a comment. In D87188#2447025 , @craig.topper wrote: > In D87188#2446096 , @spatel wrote: > >> In D87188#2445506

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

craig.topper added a comment. In D87188#2446096 , @spatel wrote: > In D87188#2445506 , @lebedev.ri > wrote: > >> Partial rebase (without updating test coverage) > > Thanks for reducing! > If I'm seeing it

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

spatel added a comment. In D87188#2445506 , @lebedev.ri wrote: > Partial rebase (without updating test coverage) Thanks for reducing! If I'm seeing it correctly, the codegen looks fine - the difference is in the IR icmp predicate changing from `slt` to

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

lebedev.ri updated this revision to Diff 310855. lebedev.ri added a comment. This revision is now accepted and ready to land. Partial rebase (without updating test coverage) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87188/new/

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

mstorsjo added a comment. In D87188#2445414 , @lebedev.ri wrote: > @mstorsjo please can you be more specific what kind of tests are starting to > fail? > Do those tests just check that the code compiled into an identical assembly? No, it's video/audio

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

lebedev.ri commandeered this revision. lebedev.ri edited reviewers, added: nikic; removed: lebedev.ri. lebedev.ri added a comment. @mstorsjo please can you be more specific what kind of tests are starting to fail? Do those tests just check that the code compiled into an identical assembly? I

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

nikic abandoned this revision. nikic added a comment. This needs someone with access to AArch64 hardware to look into https://reviews.llvm.org/D87188#2281093 to make progress. I don't have any AArch64 hardware, and judging by the time it takes to build cmake on an AArch64 machine in the GCC

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

nikic updated this revision to Diff 305232. nikic added a comment. Rebase. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87188/new/ https://reviews.llvm.org/D87188 Files: clang/test/CodeGen/builtins-wasm.c llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

nikic added a comment. In D87188#2385619 , @spatel wrote: > D90554 / f7eac51b9b > > I think that change makes this patch ready to try again. If we do

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

lebedev.ri added inline comments. Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1070 InstCombinerImpl ) { if (!Cmp.hasOneUse() || !isa(Cmp.getOperand(1))) return nullptr; nikic wrote: > nikic

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

spatel added a comment. In D87188#2348343 , @spatel wrote: > In D87188#2348326 , @nikic wrote: > >> Reopening this so we don't forget... >> >> I believe @spatel is working on the cost modelling. I did not have much

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

spatel added a comment. In D87188#2348326 , @nikic wrote: > Reopening this so we don't forget... > > I believe @spatel is working on the cost modelling. I did not have much luck > tracking down the miscompile, at least did not spot anything incriminating

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

nikic reopened this revision. nikic added a comment. This revision is now accepted and ready to land. Reopening this so we don't forget... I believe @spatel is working on the cost modelling. I did not have much luck tracking down the miscompile, at least did not spot anything incriminating in

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

mstorsjo added a comment. In D87188#2282502 , @nikic wrote: > Based on an initial look, the changes in comparison predicates here are > probably a red herring. If I understand right, those predicates are switching > from signed to unsigned comparison

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

nikic added a comment. In D87188#2281093 , @mstorsjo wrote: > This broke a few tests for me (generating code that now gives the fail result > at runtime). > > I'm not entirely sure which bit is the culprit, but the difference in output > (that breaks

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

spatel added a comment. In D87188#2281957 , @sanwou01 wrote: > I know this has already been reverted but just FYI that I've bisected a ~2% > regression in SPEC2017 x264_r on AArch64 to this commit. Presumably this is > due to the extra unrolling / cost

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

sanwou01 added a comment. I know this has already been reverted but just FYI that I've bisected a ~2% regression in SPEC2017 x264_r on AArch64 to this commit. Presumably this is due to the extra unrolling / cost modelling issue already mentioned? Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

mstorsjo added a comment. This broke a few tests for me (generating code that now gives the fail result at runtime). I'm not entirely sure which bit is the culprit, but the difference in output (that breaks tests) for one object file is available at

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

nikic added a comment. In D87188#2277728 , @spatel wrote: > LGTM - I think we should give this a try as-is (with the one-use check still > there), see if anything regresses, then ease/remove the use check as a > follow-on. Okay, let's do that. > As

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

This revision was automatically updated to reflect the committed changes. Closed by commit rG05d4c4ebc2fb: [InstCombine] Canonicalize SPF_ABS to abs intrinc (authored by nikic). Herald added a project: clang. Herald added a subscriber: cfe-commits. Changed prior to commit: