[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-13 Thread via cfe-commits
goldsteinn wrote: ping https://github.com/llvm/llvm-project/pull/82404 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-13 Thread Nikita Popov via cfe-commits
https://github.com/nikic approved this pull request. https://github.com/llvm/llvm-project/pull/82404 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-13 Thread via cfe-commits
goldsteinn wrote: @nikic, what do you think about adding `nneg` for `uitofp`? https://github.com/llvm/llvm-project/pull/82404 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-13 Thread Nikita Popov via cfe-commits
nikic wrote: @goldsteinn Doesn't seem worthwhile. https://github.com/llvm/llvm-project/pull/82404 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-13 Thread via cfe-commits
https://github.com/goldsteinn closed https://github.com/llvm/llvm-project/pull/82404 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-14 Thread Artem Belevich via cfe-commits
Artem-B wrote: We happen have a back-end where we do not have conversion instructions between unsigned int and FP, so this patch complicates things. Would it make sense to enable this canonicalization only if the target wants it? https://github.com/llvm/llvm-project/pull/82404 ___

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-14 Thread via cfe-commits
goldsteinn wrote: > We happen have a back-end where we do not have conversion instructions > between unsigned int and FP, so this patch complicates things. Would it make > sense to enable this canonicalization only if the target wants it? We try to keep InstCombine as target agnostic as possib

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-14 Thread Nikita Popov via cfe-commits
nikic wrote: I don't think we really have a strong motivation for this change beyond "having a canonical form is usually good", so if it's causing issues for some targets, then probably just not doing it is fine. But if we do want to keep it, then yeah, this would be a reasonable motivation fo

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-14 Thread via cfe-commits
goldsteinn wrote: > I don't think we really have a strong motivation for this change beyond > "having a canonical form is usually good", so if it's causing issues for some > targets, then probably just not doing it is fine. But if we do want to keep > it, then yeah, this would be a reasonable

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-19 Thread via cfe-commits
asmok-g wrote: Heads up: we noticed at google that this is causing the following test to fail: https://github.com/google/swiftshader/blob/master/tests/ReactorUnitTests/ReactorUnitTests.cpp#L1312 I need to put a more proper reproducer, but thought that at least posting the heads-up might be fas

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-19 Thread via cfe-commits
goldsteinn wrote: Ill revert this. I'll re-post if I get around to adding a flag. Any chance you can still create a repro of the bug your seeing for if I re-post? https://github.com/llvm/llvm-project/pull/82404 ___ cfe-commits mailing list cfe-commits@

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-19 Thread via cfe-commits
goldsteinn wrote: > Heads up: we noticed at google that this is causing the following test to > fail: > > https://github.com/google/swiftshader/blob/master/tests/ReactorUnitTests/ReactorUnitTests.cpp#L1312 > > I need to put a more proper reproducer, but thought that at least posting the > hea

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-20 Thread via cfe-commits
alexfh wrote: Apart from the correctness issues, we've seen some regressions on various benchmarks from LLVM Test Suite after this patch. Specifically, around 3-5% regression on x86-64 in various metrics of the [Interpolation](https://github.com/llvm/llvm-test-suite/tree/main/MicroBenchmarks/I

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-20 Thread via cfe-commits
goldsteinn wrote: > Apart from the correctness issues, we've seen some regressions on various > benchmarks from LLVM Test Suite after this patch. Specifically, around 3-5% > regression on x86-64 in various metrics of the > [Interpolation](https://github.com/llvm/llvm-test-suite/tree/main/Micro

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-20 Thread via cfe-commits
asmok-g wrote: Well, I'm not sure how proper that wold be as a reproducer, I extracted the mentioned test to a program: ``` #include #include "third_party/swiftshader/src/Reactor/Coroutine.hpp" #include "third_party/swiftshader/src/Reactor/Reactor.hpp" using float4 = float[4]; using int4 = i

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-20 Thread via cfe-commits
goldsteinn wrote: > Well, I'm not sure how proper that wold be as a reproducer, > > I extracted the mentioned test to a program: > > ``` > #include > > #include "third_party/swiftshader/src/Reactor/Coroutine.hpp" > #include "third_party/swiftshader/src/Reactor/Reactor.hpp" > > using float4 =

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-20 Thread Yingwei Zheng via cfe-commits
dtcxzyw wrote: > Apart from the correctness issues, we've seen some regressions on various > benchmarks from LLVM Test Suite after this patch. Specifically, around 3-5% > regression on x86-64 in various metrics of the > [Interpolation](https://github.com/llvm/llvm-test-suite/tree/main/MicroBen

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-21 Thread Craig Topper via cfe-commits
topperc wrote: > > Apart from the correctness issues, we've seen some regressions on various > > benchmarks from LLVM Test Suite after this patch. Specifically, around 3-5% > > regression on x86-64 in various metrics of the > > [Interpolation](https://github.com/llvm/llvm-test-suite/tree/main/

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-21 Thread via cfe-commits
goldsteinn wrote: See: https://github.com/llvm/llvm-project/pull/86141 which adds `nneg` flag to IR. Once that gets in I have patches for instcombine,cvp,sccp to do the transform. https://github.com/llvm/llvm-project/pull/82404 ___ cfe-commits mailin

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-21 Thread via cfe-commits
goldsteinn wrote: > > Well, I'm not sure how proper that wold be as a reproducer, > > I extracted the mentioned test to a program: > > ``` > > #include > > > > #include "third_party/swiftshader/src/Reactor/Coroutine.hpp" > > #include "third_party/swiftshader/src/Reactor/Reactor.hpp" > > > > us

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-21 Thread Craig Topper via cfe-commits
topperc wrote: > > > Well, I'm not sure how proper that wold be as a reproducer, > > > I extracted the mentioned test to a program: > > > ``` > > > #include > > > > > > #include "third_party/swiftshader/src/Reactor/Coroutine.hpp" > > > #include "third_party/swiftshader/src/Reactor/Reactor.hpp"

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-10 Thread via cfe-commits
https://github.com/goldsteinn updated https://github.com/llvm/llvm-project/pull/82404 >From 08fddf10b8d78f5cf866dcb687009f20cfb85bd3 Mon Sep 17 00:00:00 2001 From: Noah Goldstein Date: Tue, 20 Feb 2024 12:51:02 -0600 Subject: [PATCH] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x

[clang] [llvm] [InstCombine] Canonicalize `(sitofp x)` -> `(uitofp x)` if `x >= 0` (PR #82404)

2024-03-10 Thread via cfe-commits
goldsteinn wrote: > It looks like you need to update `Headers/__clang_hip_math.hip` in clang. Fixed. Thats what I get for not running clang tests :/ https://github.com/llvm/llvm-project/pull/82404 ___ cfe-commits mailing list cfe-commits@lists.llvm.or