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,120 msx64 debian > AddressSanitizer-x86_64-linux-dynamic.TestCases::scariness_score_test.cpp
Script: -- : 'RUN: at line 4'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -shared-libasan -O0 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/scariness_score_test.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxDynamicConfig/TestCases/Output/scariness_score_test.cpp.tmp
60,070 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 ; /usr/bin/cmake --build . --target check-standalone | tee /var/lib/buildkite-agent/builds/llvm-project/build/tools/mlir/test/Examples/standalone/Output/test.toy.tmp | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/mlir/test/Examples/standalone/test.toy

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
9433

Why is the last SDLoc from N0?

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

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
9433

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
9433

I'll take a look

RKSimon added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9433
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.