This is an archive of the discontinued LLVM Phabricator instance.

[AVR] Do not use R0/R1 on avrtiny
ClosedPublic

Authored by aykevl on Nov 23 2022, 8:42 AM.

Details

Summary

This patch makes sure the compiler uses R16/R17 on avrtiny (attiny10 etc) instead of R0/R1.

Some notes:

  • For the NEGW and ROLB instructions, it adds an explicit zero register. This is necessary because the zero register is different on avrtiny (and InstrInfo Uses lines need a fixed register).
  • Not entirely sure about putting all tests in features/avr-tiny.ll, but it doesn't seem like the "target-cpu"="attiny10" attribute works.

Patch was based on D138529, but this can be changed easily.

Diff Detail

Event Timeline

aykevl created this revision.Nov 23 2022, 8:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2022, 8:42 AM
Herald added subscribers: Jim, hiraditya. · View Herald Transcript
aykevl requested review of this revision.Nov 23 2022, 8:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2022, 8:42 AM
aykevl edited the summary of this revision. (Show Details)Nov 23 2022, 8:42 AM
aykevl edited the summary of this revision. (Show Details)Nov 23 2022, 8:43 AM

You can select https://reviews.llvm.org/D138529 as a parent of current patch, then you won't get a patch application failed error.

benshi001 added inline comments.Nov 25 2022, 12:42 AM
llvm/lib/Target/AVR/AVRFrameLowering.cpp
155

I do see avr-gcc save/restore the zero-reg on avrtiny interrupt handlers. Shall we conform with avr-gcc ?

llvm/test/CodeGen/AVR/features/avr-tiny.ll
2

How about also testing atmega328, as a contrast to avrtiny ?

benshi001 added inline comments.Nov 25 2022, 12:45 AM
llvm/test/CodeGen/AVR/features/avr-tiny.ll
60

I do see r17 is pushed/poped by avr-gcc in this case.

// avr-gcc test.c -O3 -Wall -mmcu=attiny10

#include <avr/interrupt.h>

void foo(void);

ISR(BADISR_vect)
{
        foo();
}
aykevl updated this revision to Diff 478096.Nov 27 2022, 7:55 AM
aykevl edited the summary of this revision. (Show Details)
  • fix zero register save/restore in interrupts
  • also test on atmega328p
aykevl updated this revision to Diff 478102.Nov 27 2022, 7:57 AM
aykevl edited the summary of this revision. (Show Details)
  • add explicit zero register to NEGW and ROLB

I have updated the patch. You are correct about the zero register in interrupts, I must have incorrectly remembered the other behavior. Unfortunately, fixing that also meant changing NEGW and ROLB.
I still think it should be safe to omit saving/restoring R17 but I might do that in a separate patch.

benshi001 accepted this revision.Nov 27 2022, 11:24 PM

LGTM. Thanks.

This revision is now accepted and ready to land.Nov 27 2022, 11:24 PM
This revision was landed with ongoing or failed builds.Nov 28 2022, 9:06 AM
This revision was automatically updated to reflect the committed changes.