This is an archive of the discontinued LLVM Phabricator instance.

[AVR] Reject/Reserve R0~R15 on AVRTiny
ClosedPublic

Authored by benshi001 on Mar 15 2022, 12:22 AM.

Diff Detail

Event Timeline

benshi001 created this revision.Mar 15 2022, 12:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2022, 12:22 AM
Herald added subscribers: Jim, hiraditya. · View Herald Transcript
benshi001 requested review of this revision.Mar 15 2022, 12:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2022, 12:22 AM
benshi001 updated this revision to Diff 415376.Mar 15 2022, 3:46 AM
This comment was removed by benshi001.
benshi001 updated this revision to Diff 415674.Mar 15 2022, 7:50 PM

The build failure is about libFuzzer.libFuzzer::large.test , not caused by my patch.

aykevl added inline comments.Mar 22 2022, 5:21 PM
llvm/lib/Target/AVR/AVRCallingConv.td
41

I don't think this is needed. Reserved registers are not part of the regular calling convention anyway.

llvm/lib/Target/AVR/AVRISelLowering.cpp
1057–1060

I think it would make sense to use a vector here, to make the code safer. Take a look here to see the recommended vector types for use in LLVM: https://llvm.org/docs/ProgrammersManual.html#sequential-containers-std-vector-std-list-etc

llvm/lib/Target/AVR/AVRRegisterInfo.cpp
61–63

I think it would make sense to change these hardcoded R0/R1/R1R0 registers to properties of AVRSubtarget (a bit like getIORegisterOffset for example). This would make the code below a bit more sensible: it could just reserve R0-R15 instead of R2-R17 which looks a bit strange.
Such a change should probably need to be done in a separate patch, as a cleanup before this patch. Such a patch could also fix the hardcoded R0/R1 registers in AVRFrameLowering.cpp, AVRExpandPseudoInsts.cpp, etc.

aykevl added inline comments.Mar 22 2022, 5:37 PM
llvm/lib/Target/AVR/AVRRegisterInfo.cpp
61–63

It looks like you already added these getters to AVRSubtarget in D119807.

benshi001 added inline comments.Mar 22 2022, 11:47 PM
llvm/lib/Target/AVR/AVRRegisterInfo.cpp
61–63

Thanks for your suggestion.

But I do not think reserve R0-R15 looks good. Since R0&R1 are always reserved on both avr and avrtiny. While R2-R15 are only reserved on avrtiny. It looks strange like

if (STI.hasTinyEncoding()) {
    for (unsigned Reg = AVR::R0; Reg <= AVR::R17; Reg++)
      Reserved.set(Reg);
else {
      Reserved.set(R0);
      Reserved.set(R1);
}

My points are

  1. R0 and R1 are always reserved, no matter avr or avrtiny.
  2. R2-R17 are reserved only on avrtiny, while are available on avr.
benshi001 added inline comments.Mar 23 2022, 1:21 AM
llvm/lib/Target/AVR/AVRRegisterInfo.cpp
61–63

One more issue of doing

Reserved.set(getTempReg());
Reserved.set(getZeroReg());

is that, we also have to reserve 16-bit registers R0R1/R16R15/R17R16/R18/R17 on avrtiny. It makes sence to define getTempReg() and getZeroRegs(), but not for something like getTinyReservedRegs().

benshi001 updated this revision to Diff 417540.Mar 23 2022, 2:25 AM
benshi001 marked 2 inline comments as done.
benshi001 updated this revision to Diff 417541.Mar 23 2022, 2:31 AM
benshi001 set the repository for this revision to rG LLVM Github Monorepo.
benshi001 marked an inline comment as done.Mar 23 2022, 2:53 AM
benshi001 added inline comments.
llvm/lib/Target/AVR/AVRCallingConv.td
41

Unfortunately this is a must. Since R19 and R18 are callee saved registers on avrtiny, according to https://gcc.gnu.org/wiki/avr-gcc#Reduced_Tiny, though they are not so on normal avr.

So we should explicitly mark R19/R18 as callee saved for avrtiny, and exclude R17/R16 from interrupt/singal_handler saved list.

benshi001 updated this revision to Diff 417546.Mar 23 2022, 2:59 AM
benshi001 marked an inline comment as done.
aykevl accepted this revision.Mar 23 2022, 10:06 AM
aykevl added inline comments.
llvm/lib/Target/AVR/AVRCallingConv.td
41

Ah, I see. You are correct.

This revision is now accepted and ready to land.Mar 23 2022, 10:06 AM
This revision was landed with ongoing or failed builds.Mar 23 2022, 7:34 PM
This revision was automatically updated to reflect the committed changes.
aykevl added inline comments.Nov 15 2022, 5:27 AM
llvm/lib/Target/AVR/AVRISelLowering.cpp
1157–1158

While updating D131867 I found there is a bug in this code: RegList16 has 6 elements while RegIdx below can be up to 7. This results in an array out of bounds error.
I believe the issue here is that return values greater than 4 bytes should be returned via the stack on avrtiny, see: https://godbolt.org/z/GzxGezE81

benshi001 marked an inline comment as done.Nov 16 2022, 5:21 AM
benshi001 added inline comments.
llvm/lib/Target/AVR/AVRISelLowering.cpp
1157–1158

Thanks for your information. I have made a patch to fix that.

https://reviews.llvm.org/D138125

Beside changes in the clang side, some extra work is need on the backend, and I will do that later this week.

benshi001 marked an inline comment as done.Nov 17 2022, 4:50 AM
benshi001 added inline comments.
llvm/lib/Target/AVR/AVRISelLowering.cpp
1157–1158

The corresponding fix in the backend is here, https://reviews.llvm.org/D138201