Page MenuHomePhabricator

[InstCombine] Add constant combines for `(urem/srem (shl X, Y), (shl X, Z))`
Needs ReviewPublic

Authored by goldstein.w.n on Feb 16 2023, 3:29 PM.

Details

Summary

Forked from D142901 to deduce more nsw/nuw flag for the output
shl.

We can handle the following cases + some nsw/nuw flags:

The rationale for doing this all in InstCombine rather than handling
the constant shl cases in InstSimplify is we often create a new
instruction because we are able to deduce more nsw/nuw flags than
the original instruction had.

Diff Detail

Unit TestsFailed

TimeTest
90 msx64 debian > Clang.Driver::linker-wrapper-image.c
Script: -- : 'RUN: at line 5'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/linker-wrapper-image.c -triple x86_64-unknown-linux-gnu -emit-obj -o /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Driver/Output/linker-wrapper-image.c.tmp.elf.o
190 msx64 debian > Clang.Driver::linker-wrapper-libs.c
Script: -- : 'RUN: at line 7'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/linker-wrapper-libs.c -triple x86_64-unknown-linux-gnu -emit-obj -o /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Driver/Output/linker-wrapper-libs.c.tmp.elf.o
170 msx64 debian > Clang.Driver::linker-wrapper.c
Script: -- : 'RUN: at line 5'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/linker-wrapper.c -triple x86_64-unknown-linux-gnu -emit-obj -o /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Driver/Output/linker-wrapper.c.tmp.elf.o
60 msx64 debian > Clang.Driver::offload-packager.c
Script: -- : 'RUN: at line 6'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/offload-packager.c -triple x86_64-unknown-linux-gnu -emit-obj -o /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Driver/Output/offload-packager.c.tmp.elf.o
60,050 msx64 debian > MLIR.Examples/standalone::test.toy
Script: -- : 'RUN: at line 1'; /usr/bin/cmake /var/lib/buildkite-agent/builds/llvm-project/mlir/examples/standalone -G "Ninja" -DCMAKE_CXX_COMPILER=/usr/bin/clang++ -DCMAKE_C_COMPILER=/usr/bin/clang -DLLVM_ENABLE_LIBCXX=OFF -DMLIR_DIR=/var/lib/buildkite-agent/builds/llvm-project/build/lib/cmake/mlir -DLLVM_USE_LINKER=lld -DPython3_EXECUTABLE="/usr/bin/python3.9"

Event Timeline

goldstein.w.n created this revision.Feb 16 2023, 3:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2023, 3:29 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
goldstein.w.n requested review of this revision.Feb 16 2023, 3:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2023, 3:29 PM

Use match instead of handwritten logic

sdesmalen added inline comments.Tue, Mar 14, 2:23 AM
llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
1707

It might be a bit easier to follow, if you explicitly do the scaling while doing the matching, i.e.

APInt Y, Z;
const APInt *MatchY = nullptr, *MatchZ = nullptr;

// Match and normalise shift-amounts to multiplications
if (match(Op0, m_c_Mul(m_Value(X), m_APInt(MatchY)))) {
  Y = *MatchY;
  if (match(Op1, m_Shl(m_Specific(X), m_APInt(MatchZ))))
    //  rem(mul(x, y), shl(x, z))
    Z = APInt(X->getType()->getScalarSizeInBits(), 1).shl(*MatchZ);
  else if (match(Op1, m_c_Mul(m_Specific(X), m_APInt(MatchZ))))
    //  rem(mul(x, y), mul(x, z))
    Z = *MatchZ;
} else if (match(Op0, m_Shl(m_Value(X), m_APInt(MatchY)))) {
  Y = APInt(X->getType()->getScalarSizeInBits(), 1).shl(*MatchY);
  if (match(Op1, m_Shl(m_Specific(X), m_APInt(MatchZ))))
    //  rem(shl(x, y), shl(x, z))
    Z = APInt(X->getType()->getScalarSizeInBits(), 1).shl(*MatchZ);
  else if (match(Op1, m_c_Mul(m_Specific(X), m_APInt(MatchZ))))
    //  rem(shl(x, y), mul(x, z))
    Z = *MatchZ;
}

if (!MatchY || !MatchZ)
  return nullptr;
1722–1724

I can't see this case being tested anywhere. I'd suggest singling this case out from the other logic and handling it separately. Maybe you can move that to another patch (or make it part of D143417, which tries to handle a similar case). That makes this patch a bit simpler and avoids the need for GetBinOpOut.