Page MenuHomePhabricator

[AVR] Fix expanding MOVW for overlapping registers
ClosedPublic

Authored by Patryk27 on Jun 25 2022, 10:24 AM.

Details

Summary

Sometimes the codegen emits a COPY with overlapping registers, such as
this one:

$r25r24 = COPY $r24r23

Our current expansion of such COPY would be wrong, since it'd start
with the lower register first, with the second mov assuming the
registers are not modified in-between:

mov r24, r23
mov r25, r24

(i.e. that's effectively mov r25, r23, which is _ayy ayy bad_.)

This patch improves the expansion by making it detect whether the
registers are overlapping and if so, expanding from the high register
first:

mov r25, r24
mov r24, r23

Because our registers are always paired in descending order (e.g.
there's r25r24, but no r24r25), I think it'd be safe to go with the
high-to-low expansion always (not only if the registers overlap), but
that makes the output a bit more difficult to follow and would require
adjusting the existing tests -- so I went with the more safer & simpler
route.

In the wild, this was found here:
https://github.com/rust-lang/rust/issues/98167.

Diff Detail

Unit TestsFailed

TimeTest
60,130 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,090 msx64 debian > AddressSanitizer-x86_64-linux.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 -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_64LinuxConfig/TestCases/Output/scariness_score_test.cpp.tmp

Event Timeline

Patryk27 created this revision.Jun 25 2022, 10:24 AM
Patryk27 requested review of this revision.Jun 25 2022, 10:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2022, 10:24 AM
benshi001 added inline comments.Jun 25 2022, 8:43 PM
llvm/lib/Target/AVR/AVRInstrInfo.cpp
61

Using name CopyHasOverlappingRegs maybe confusing, for the case DestHi == SrcLow. Though there is overlapping for r25r24 -> r24r23, we have to still copy lower byte first.

So I suggest

if (DestLo == SrcHi) {
  ...
} else {
  ...
}

And the long comment is unnecessary, since the logic is clear enough to understand.

llvm/test/CodeGen/AVR/pseudo/COPY.mir
31

Are these two implicit killed in the two RCALLk necessary ?

Patryk27 updated this revision to Diff 440046.Jun 26 2022, 1:10 AM

Apply code review changes

Patryk27 marked 2 inline comments as done.Jun 26 2022, 1:11 AM
Patryk27 added inline comments.
llvm/test/CodeGen/AVR/pseudo/COPY.mir
31

Without any instruction that actually uses the result of that COPY, LLVM seems to generate no code whatsoever - I've thought about RCALLk as a simple way of telling LLVM "trust me, that register is read later".

Those don't have to be killed though, so to simplify the test, I've just changed them to RCALLk @foo, implicit $r24r23.

benshi001 accepted this revision.Jun 26 2022, 2:09 AM
This revision is now accepted and ready to land.Jun 26 2022, 2:09 AM
This revision was landed with ongoing or failed builds.Jun 26 2022, 2:32 AM
This revision was automatically updated to reflect the committed changes.