This is an archive of the discontinued LLVM Phabricator instance.

[AVR][MC] Generate section '.progmemX.data' for extended flash banks
ClosedPublic

Authored by benshi001 on Dec 18 2021, 2:05 AM.

Diff Detail

Event Timeline

benshi001 created this revision.Dec 18 2021, 2:05 AM
benshi001 requested review of this revision.Dec 18 2021, 2:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 18 2021, 2:05 AM
benshi001 updated this revision to Diff 395652.Dec 21 2021, 4:46 AM
benshi001 updated this revision to Diff 395811.Dec 22 2021, 1:42 AM

Some design notes

  1. Input address space 1 corresponds to __flash (ordinrary flash < 64KB), addrspace 2 -> __flash1 (extended program memory bank1, 64-128KB), ..., addrspace 6 -> __flash5 (extended program memory bank 5, 320KB-384KB). And all other addrspaces fall back to SRAM.
  1. For MCU with neither FeatureLPM nor FeatureELPM, all input address spaces fall back to SRAM, and no LPM/ELPM instruction will be generated.
  1. For MCU with only FeatureLPM but not FeatureELPM, all access to address space 2-6 fall back to ordinary program space (0-64KB), and only LPM will be generated, and no ELPM will be generated.

I wonder whether we should generate an error instead of falling back to other memory areas? That would be more consistent with avr-gcc and I think that would also be more correct.

llvm/test/CodeGen/AVR/sections.ll
32

Why not check for what it should be, instead of what it should not be? Something like this (untested):

; CHECK8515: .section .data,"a"
benshi001 updated this revision to Diff 398575.Jan 10 2022, 4:39 AM
benshi001 marked an inline comment as done.Jan 10 2022, 4:42 AM

I wonder whether we should generate an error instead of falling back to other memory areas? That would be more consistent with avr-gcc and I think that would also be more correct.

Actually an error will be reported by clang as in https://reviews.llvm.org/D115982, which is indeed consistent with avr-gcc.

And in current patch, a brief error will be reported too (though the location information needs to be filled by future patches).

benshi001 updated this revision to Diff 398596.Jan 10 2022, 5:47 AM
benshi001 updated this revision to Diff 398827.Jan 10 2022, 8:36 PM
aykevl accepted this revision.Jan 19 2022, 3:10 PM

Looks good to me! A few suggestions below (which aren't necessary, just ideas).

llvm/lib/Target/AVR/AVR.h
49

Suggestion: you could also call this NumAddrSpaces because it contains the number of address spaces.

llvm/lib/Target/AVR/AVRTargetObjectFile.cpp
45–62

To be honest, I don't think this error checking is needed. If there is a section in address space 3 for example, but this is compiled for an atmega328p (which only supports address space 0 and 1), then the necessary ELPM instructions will already cause a compiler error. Having .progmem2.data that the program can't access doesn't harm.

But it doesn't harm either, so feel free to leave it in.

This revision is now accepted and ready to land.Jan 19 2022, 3:10 PM
benshi001 marked an inline comment as done.Jan 19 2022, 6:36 PM
benshi001 added inline comments.
llvm/lib/Target/AVR/AVR.h
49

I will change it to NumAddrSpaces while commiting.

benshi001 marked an inline comment as done.Jan 19 2022, 6:36 PM