Page MenuHomePhabricator

[AVR] Fix expanding MOVW for overlapping registers

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



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

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

In the wild, this was found here:

Diff Detail

Unit TestsFailed

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

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.


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.

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.