Page MenuHomePhabricator

[AVR] Only push and clear R1 in interrupts when necessary
AcceptedPublic

Authored by aykevl on Jan 16 2022, 4:40 AM.

Details

Summary

r1 is a reserved register, but LLVM gives the APIs to know when it is used or not. So this patch uses these APIs to only save/clear/restore r1 in interrupts when necessary.

The main issue here was getting inline assembly to work. One could argue that this is the job of Clang, but for consistency I've made sure that r1 is always usable in inline assembly even if that means clearing it when it might not be needed.

Information on inline assembly in AVR can be found here:

https://www.nongnu.org/avr-libc/user-manual/inline_asm.html#asm_code

Essentially, this seems to suggest that r1 can be freely used in avr-gcc inline assembly, even without specifying it as an input operand.


This patch depends on D117425.

Diff Detail

Unit TestsFailed

TimeTest
60,070 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
60,030 msx64 debian > libFuzzer.libFuzzer::fuzzer-leak.test
Script: -- : 'RUN: at line 3'; /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/LeakTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/fuzzer-leak.test.tmp-LeakTest
60,070 msx64 debian > libFuzzer.libFuzzer::large.test
Script: -- : 'RUN: at line 3'; /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/LargeTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/large.test.tmp-LargeTest
60,030 msx64 debian > libFuzzer.libFuzzer::minimize_crash.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/NullDerefTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/minimize_crash.test.tmp-NullDerefTest
60,030 msx64 debian > libFuzzer.libFuzzer::out-of-process-fuzz.test
Script: -- : 'RUN: at line 2'; rm -rf /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/out-of-process-fuzz.test.tmp && mkdir /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/out-of-process-fuzz.test.tmp
View Full Test Results (6 Failed)

Event Timeline

aykevl created this revision.Jan 16 2022, 4:40 AM
aykevl requested review of this revision.Jan 16 2022, 4:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2022, 4:40 AM
aykevl added inline comments.Jan 16 2022, 4:43 AM
llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
1322

This was a bug.

llvm/lib/Target/AVR/AVRISelLowering.cpp
822

This is a bit of a hack, but I couldn't think of a better way.

benshi001 added inline comments.Feb 14 2022, 12:24 AM
llvm/lib/Target/AVR/AVRISelLowering.cpp
844

I think the following form is more readable:

for (unsigned I = 0; I < N->getNumOperands(); I++) {
  SDValue Operand = N->getOperand(I);
  if (Operand.getValueType() == MVT::Glue)
    // The glue operand always needs to be at the end, so we need to treat it
    // specially.
    Glue = Operand;
  else
    Ops.push_back(Operand);
}

This patch failed to be applied, so you need to rebase ?

benshi001 added inline comments.Feb 14 2022, 1:17 AM
llvm/lib/Target/AVR/AVRISelLowering.cpp
822

I think this way is OK enough.

836

The flag of the implicit R1 register operand is better.

837

the implicit R1 register operand itself.

aykevl updated this revision to Diff 418203.Mar 25 2022, 6:18 AM

@benshi001 I've rebased the patch and updated it with your suggestions.

Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2022, 6:18 AM
benshi001 accepted this revision.Mar 25 2022, 9:01 AM
benshi001 added inline comments.
llvm/lib/Target/AVR/AVRISelDAGToDAG.cpp
460

I am OK with current form. Just one more suggestion, you can add a new case of icall, current tests only contains a direct call but no indirect call. And you can add the icall test either in this patch or a new patch.

This revision is now accepted and ready to land.Mar 25 2022, 9:01 AM
benshi001 added inline comments.Mar 26 2022, 6:41 AM
llvm/lib/Target/AVR/AVRISelDAGToDAG.cpp
460

I have debugged your code, and I find this line is unnecessary.

Since there is already such a line in AVRISelLowering.cpp / LowerCall():

// Add argument registers to the end of the list so that they are known live
// into the call.
for (auto Reg : RegsToPass) {
  Ops.push_back(DAG.getRegister(Reg.first, Reg.second.getValueType()));
}

// The R1 register must be passed as an implicit register so that R1 is
// correctly zeroed in interrupts.
Ops.push_back(DAG.getRegister(AVR::R1, MVT::i8));

That line in LowerCall is always executed, no matter direct call or indirect call, so the implicit R1 will be added twice for indirect calls.

benshi001 requested changes to this revision.Mar 26 2022, 6:59 AM
This revision now requires changes to proceed.Mar 26 2022, 6:59 AM
benshi001 accepted this revision.Mar 26 2022, 7:00 AM
This revision is now accepted and ready to land.Mar 26 2022, 7:00 AM
benshi001 added inline comments.Mar 26 2022, 7:01 AM
llvm/lib/Target/AVR/AVRISelDAGToDAG.cpp
460

I still accept your patch, and you can delete this line in AVRDAGToDAGISel::select<AVRISD::CALL> when commit.

benshi001 added inline comments.Wed, May 4, 8:24 PM
llvm/lib/Target/AVR/AVRISelDAGToDAG.cpp
460

@aykevl , do you have any different opinion on my comments? I suggest you commit this ASAP.