This is a patch that adds folding of two logical and/ors that share one variable:
a && (a && b) -> a && b
a && (a & b) -> a && b
...
This is towards removing the poison-unsafe select optimization (D93065 has more context).
Differential D96945
[InstCombine] Add simplification of two logical and/ors aqjune on Feb 18 2021, 1:48 AM. Authored by
Details This is a patch that adds folding of two logical and/ors that share one variable: a && (a && b) -> a && b This is towards removing the poison-unsafe select optimization (D93065 has more context).
Diff Detail
Event Timeline
Comment Actions HI, A bisect seems to show this patch causing the test failures we see on our two-stage clang builders (https://luci-milo.appspot.com/p/fuchsia/builders/prod/clang-linux-x64/b8853364937280028160?): ******************** LLVM :: Transforms/InstCombine/2011-05-28-swapmulsub.ll LLVM :: Transforms/InstCombine/bitcast-bigendian.ll LLVM :: Transforms/InstCombine/bitcast-inseltpoison.ll LLVM :: Transforms/InstCombine/bitcast.ll LLVM :: Transforms/InstCombine/icmp-custom-dl.ll LLVM :: Transforms/InstCombine/icmp.ll LLVM :: Transforms/InstCombine/shift-amount-reassociation-in-bittest-with-truncation-shl.ll LLVM :: Transforms/InstCombine/shift-amount-reassociation-with-truncation-shl.ll LLVM :: Transforms/InstCombine/sub-gep.ll LLVM :: Transforms/InstCombine/sub.ll LLVM :: Transforms/InstCombine/trunc-inseltpoison.ll LLVM :: Transforms/InstCombine/trunc.ll Would you mind taking a look and sending out a fix or reverting? Thanks. Comment Actions Yep, I reverted this patch. Could you post the commandline options that is used to build LLVM? Comment Actions Thanks for the followup. Can do. Let me see if I can minimize the reproducer/reduce the number of cmake flags needed for ease first since the two-stage build has dependencies on libxml2 and zlib. While I'm doing that, if you already have a fix, what I can do is patch it locally and get back to you on if it also fixes the issues we see. Comment Actions Ok, so the cmake invocation for the second stage build is: cmake "-DCMAKE_EXE_LINKER_FLAGS=-ldl -lpthread" "-DCMAKE_MODULE_LINKER_FLAGS=-ldl -lpthread" "-DCMAKE_SHARED_LINKER_FLAGS=-ldl -lpthread" -DCMAKE_SYSROOT=/usr/local/google/home/leonardchan/sysroot/linux-amd64 -DLLVM_ENABLE_LLD=ON -DLLVM_ENABLE_LTO=ON -DLLVM_EXTERNAL_CLANG_SOURCE_DIR=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-1/llvm/../clang -DLLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-1/llvm/../clang-tools-extra -DLLVM_EXTERNAL_LLD_SOURCE_DIR=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-1/llvm/../lld -DLLVM_EXTERNAL_POLLY_SOURCE_DIR=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-1/llvm/../polly -DPACKAGE_VERSION=13.0.0git -DPACKAGE_VENDOR=Fuchsia -DLLVM_VERSION_MAJOR=13 -DLLVM_VERSION_MINOR=0 -DLLVM_VERSION_PATCH=0 -DCLANG_VERSION_MAJOR=13 -DCLANG_VERSION_MINOR=0 -DCLANG_VERSION_PATCHLEVEL=0 -DCLANG_VENDOR=Fuchsia -DLLVM_VERSION_SUFFIX=git -DLLVM_BINUTILS_INCDIR= -DCLANG_REPOSITORY_STRING= -DCMAKE_MAKE_PROGRAM=/usr/bin/ninja "-DLLVM_ENABLE_PROJECTS=clang;clang-tools-extra;lld;llvm;polly" "-DLLVM_ENABLE_RUNTIMES=compiler-rt;libcxx;libcxxabi;libunwind" -DLINUX_aarch64-unknown-linux-gnu_SYSROOT=/usr/local/google/home/leonardchan/sysroot/linux-arm64 -DLINUX_x86_64-unknown-linux-gnu_SYSROOT=/usr/local/google/home/leonardchan/sysroot/linux-amd64 -C /usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-1/clang/cmake/caches/Fuchsia-stage2.cmake -DCLANG_STAGE=stage2 -DCMAKE_CXX_COMPILER=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-1-master-fuchsia-toolchain/./bin/clang++ -DCMAKE_C_COMPILER=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-1-master-fuchsia-toolchain/./bin/clang -DCMAKE_ASM_COMPILER=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-1-master-fuchsia-toolchain/./bin/clang -DCMAKE_ASM_COMPILER_ID=Clang -DCMAKE_AR=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-1-master-fuchsia-toolchain/./bin/llvm-ar -DCMAKE_RANLIB=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-1-master-fuchsia-toolchain/./bin/llvm-ranlib -GNinja /usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-1/llvm /usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-1 can be replaced with whatever the path pointing to your llvm-project repository is. After this, I'm able to reproduce the failures with ninja check-llvm in my second-stage build. Let me know if I can help more/clarify on some things. Comment Actions We're still seeing some *very* strange miscompiles as a result of this patch (maybe not really this patch's fault, it could be some bad optimization that this unblocks). I'm working on a reduction right now. Comment Actions Here's a repro for the issue we're seeing. It only seems to be happening with msan enabled, so I'm not sure if this is just an msan bug, or a general miscompile that *happens* to trigger in this test when msan is enabled. The source: #include <string> #include <variant> struct a { double b; double c; std::string d; }; std::variant<std::pair<int, int>, std::string> e(a f) { double g = f.b, h = f.c; if (f.d == "y") { g *= 2.0; h *= 2.0; } int i(g); int k(h); if (i == 0) i = h; if (k == 0) k = i; if (k < i || i < 0) { std::string j; return j; } return std::make_pair(i, k); } int main() { auto j = e({1, 0, "x"}); std::get<0>(j); } To build/run it: $ clang++ \ -O2 \ -msse4.2 \ -fsanitize=memory \ -std=gnu++17 \ -stdlib=libc++ \ repro.cc \ -o bug $ ./bug terminating with uncaught exception of type std::bad_variant_access: bad_variant_access Thoughts on this example? If I follow correctly, at the if (k < i || i < 0) { check, both k and i should be 1, and that check should not trigger. Instead, it evaluates to true: the std::get<0>(j) triggers an exception, because the method return a std::string, not a std::pair after this patch. Comment Actions It indeed seems tricky to find out the root cause for this; this optimization enabled vectorization which led to the error for some reason. Comment Actions This turns out to be a bug in InstCombine. I reported it as https://llvm.org/pr49832 . Comment Actions @spatel c590a9880d7a660a1c911fce07f3d01ea18be2df fixes the unreduced test case. Thanks! Comment Actions Great! We also have essentially the same bug inside of InstSimplify, and that should be fixed with: I'm not sure exactly where we stand with respect to exposure/timing for the 12.0 release, but I've marked the patches as blockers for now. |
clang-format not found in user's PATH; not linting file.