Page MenuHomePhabricator

[LSR][RISCV] Improve test coverage for LSR in RISC-V
Needs RevisionPublic

Authored by eopXD on Apr 9 2022, 11:58 AM.

Details

Reviewers
joshua-arch1
asb
craig.topper
jrtc27
frasercrmck
reames
Group Reviewers
Restricted Project
Summary

Patch like D116735 suggest improvement for LSR, but we currently
don't have test coverage for this under RISCV. Testcases are modified
from CodeGen/X86/cases loop-strength-reduce*.ll

Diff Detail

Unit TestsFailed

TimeTest
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 ; /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
60,080 msx64 debian > ThreadSanitizer-x86_64.ThreadSanitizer-x86_64::restore_stack.cpp
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=thread -Wall -m64 -msse4.2 -gline-tables-only -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -std=c++11 -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -O1 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp && not /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp 2>&1 | FileCheck /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp
2,210 msx64 debian > libFuzzer.libFuzzer::merge_two_step.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/FullCoverageSetTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/merge_two_step.test.tmp-FullCoverageSetTest

Event Timeline

eopXD created this revision.Apr 9 2022, 11:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2022, 11:58 AM
eopXD requested review of this revision.Apr 9 2022, 11:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2022, 11:58 AM
eopXD updated this revision to Diff 421750.Apr 9 2022, 11:59 AM

Update code.

eopXD edited the summary of this revision. (Show Details)Apr 9 2022, 12:00 PM
eopXD added reviewers: jrtc27, frasercrmck.
eopXD edited the summary of this revision. (Show Details)Apr 9 2022, 12:03 PM
craig.topper added inline comments.Apr 9 2022, 8:09 PM
llvm/test/CodeGen/RISCV/loop-strength-reduce.ll
7 ↗(On Diff #421750)

Can we add signext attributes to the i32 arguments to match the ABI for rv64 and remove some extra sext.w

jrtc27 added a comment.Apr 9 2022, 9:10 PM

The test case file names are uninformative, and having both reduce-2/3 and reduce2/3 doesn't seem like a good idea

eopXD updated this revision to Diff 421798.Apr 10 2022, 11:18 AM

Update code.

  • Delete peephole optimization testcases of X86
  • Delete duplicate test case (since we are checking line-by-line)
  • Rename test cases
  • Add some descriptions into test cases
eopXD marked an inline comment as done.Apr 10 2022, 11:20 AM
eopXD added a comment.Apr 16 2022, 3:20 AM

gentle ping.

craig.topper added inline comments.Apr 18 2022, 1:09 PM
llvm/test/CodeGen/RISCV/loop-strength-reduce-add-cheaper-than-mul.ll
75

Please use the -mattr=+m to avoid this libcall

llvm/test/CodeGen/RISCV/loop-strength-reduce-ivusers.ll
49

Why is this loop substantially simpler than the RV32 version? I suspect LSR doesn't run on RV64, but that means that LSR is making the RV32 loop worse than we would get without it?

58

Are these branch on true needed?

75

This load is dead. This whole test looks overly reduced.

eopXD updated this revision to Diff 427926.May 8 2022, 4:49 AM
eopXD marked 3 inline comments as done.

Update testcase:

  • add -mattr=+m for loop-strength-reduce-add-cheaper-than-mul.ll
  • run simplifycfg for loop-strength-reduce-ivusers.ll
llvm/test/CodeGen/RISCV/loop-strength-reduce-ivusers.ll
49

Yes. LSR is generating running on RV32 and generating worse code.

58

Updated test case with simplifycfg

75

Updated test case with simplifycfg

eopXD added a subscriber: reames.May 12 2022, 10:47 AM

@reames
Since you mentioned the LSR deficiency in your public note.
You may be interested with these new test cases for the RISC-V backend.

eopXD edited the summary of this revision. (Show Details)May 12 2022, 10:48 AM
eopXD added a reviewer: Restricted Project.May 19 2022, 11:35 PM
eopXD updated this revision to Diff 430894.May 20 2022, 12:18 AM

Rebase to latest main.

reames requested changes to this revision.May 20 2022, 10:25 AM

I split off, cleaned up, and landed two of these in 923831e. It was easier to do the cleanup myself than explain what needed done.

The two I left out make heavy use of struct types. In addition to the style of simplification done in the landed ones, we should also maximally simplify the struct types involved. Want to take a shot at that? I ran out of time for this at the moment.

This revision now requires changes to proceed.May 20 2022, 10:25 AM