Page MenuHomePhabricator

[DAG] Fold (srl (shl x, c1), c2) -> and(shl/srl(x, c3), m)
ClosedPublic

Authored by RKSimon on May 17 2022, 2:18 PM.

Details

Summary

Similar to the existing (shl (srl x, c1), c2) fold

Part of the work to fix the regressions in D77804

Diff Detail

Unit TestsFailed

TimeTest
60,050 msx64 debian > Clang.CodeGenCXX::dllimport-members.cpp
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -no-opaque-pointers -no-enable-noundef-analysis -disable-llvm-passes -triple i686-windows-msvc -fms-compatibility -emit-llvm -std=c++1y -O0 -o - /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGenCXX/dllimport-members.cpp -DMSABI | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --check-prefix=MSC --check-prefix=M32 /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGenCXX/dllimport-members.cpp
60,790 msx64 debian > Clang.Driver::arm-cortex-cpus-2.c
Script: -- : 'RUN: at line 8'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -target armv8a-linux-eabi -mcpu=cortex-a53+fp16 -### -c /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/arm-cortex-cpus-2.c 2>&1 | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --check-prefix CHECK-CORTEX-A53-FP16 /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/arm-cortex-cpus-2.c
60,920 msx64 debian > Clang.Driver::fsanitize.c
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -target x86_64-linux-gnu -fsanitize=undefined -fsanitize-trap=undefined /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/fsanitize.c -### 2>&1 | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/fsanitize.c --check-prefix=CHECK-UNDEFINED-TRAP
60,150 msx64 debian > Clang.OpenMP::target_teams_distribute_parallel_for_simd_codegen_registration.cpp
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -no-opaque-pointers -verify -fopenmp -fopenmp-version=45 -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-ibm-linux-gnu -emit-llvm /var/lib/buildkite-agent/builds/llvm-project/clang/test/OpenMP/target_teams_distribute_parallel_for_simd_codegen_registration.cpp -o - | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --allow-unused-prefixes /var/lib/buildkite-agent/builds/llvm-project/clang/test/OpenMP/target_teams_distribute_parallel_for_simd_codegen_registration.cpp
61,030 msx64 debian > Clang.OpenMP::target_update_codegen.cpp
Script: -- : 'RUN: at line 6'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -no-opaque-pointers -DCK1 -verify -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -emit-llvm /var/lib/buildkite-agent/builds/llvm-project/clang/test/OpenMP/target_update_codegen.cpp -o - | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --allow-unused-prefixes /var/lib/buildkite-agent/builds/llvm-project/clang/test/OpenMP/target_update_codegen.cpp --check-prefix CK1 --check-prefix CK1-64

Event Timeline

RKSimon created this revision.May 17 2022, 2:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2022, 2:18 PM
RKSimon requested review of this revision.May 17 2022, 2:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2022, 2:18 PM
foad added a comment.Jun 13 2022, 2:08 AM

Surely the (srl (shl x, c1), c2) form is better for targets which aren't good at doing an AND-with-large-immediate-value. Is this supposed to be a canonicalization, which targets will undo later as appropriate?

Surely the (srl (shl x, c1), c2) form is better for targets which aren't good at doing an AND-with-large-immediate-value. Is this supposed to be a canonicalization, which targets will undo later as appropriate?

We have the TLI shouldFoldConstantShiftPairToMask callback to allow targets to control this.

craig.topper added inline comments.Jun 14 2022, 10:37 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9437

Why is the last SDLoc from N0?

RKSimon added inline comments.Jun 14 2022, 10:48 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9437

It was copy+pasted from the (shl (srl x, c1), c2) variant

craig.topper added inline comments.Jun 14 2022, 10:58 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9437

Is that correct? Can you swap the order of debug locs? The N0 debug loc is now after the N location in the DAG

RKSimon added inline comments.Jun 14 2022, 11:02 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9437

I'll take a look

RKSimon added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9437
RKSimon updated this revision to Diff 437103.Jun 15 2022, 3:24 AM

Fix SDLoc mismatch

RKSimon updated this revision to Diff 437916.Jun 17 2022, 8:18 AM
RKSimon retitled this revision from [DAG] Fold (srl (shl x, c1), c2) -> and(shl/srl(x, c3), m) (WIP) to [DAG] Fold (srl (shl x, c1), c2) -> and(shl/srl(x, c3), m).
RKSimon edited the summary of this revision. (Show Details)

Extend AArch64TargetLowering::shouldFoldConstantShiftPairToMask so that we don't lose the srl(shl(x,c1),c2) pair if it would likely break a UBFX pattern.

The AArch64 change sounds OK to me.

Thank @dmgreen - does anyone have more comments?

spatel accepted this revision.Jun 19 2022, 12:21 PM

LGTM - noted a close call in x86 tests, but that can be addressed if we find real regressions.

llvm/test/CodeGen/X86/shift-mask.ll
597

This and the next diff might be worse depending on uarch, so that suggests a possible refinement for the x86 TLI hook for 64-bit integer.

This revision is now accepted and ready to land.Jun 19 2022, 12:21 PM
This revision was landed with ongoing or failed builds.Jun 20 2022, 12:37 AM
This revision was automatically updated to reflect the committed changes.