Skip to content

[AVR] Handle flash RO data mapped to data space for newer devices #146244

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[AVR] Handle flash RO data mapped to data space for newer devices #146244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
137c182
[AVR] Handle mapped RO data for newer devices
tomtor Jun 27, 2025
5c32a48
Fix comment
tomtor Jul 2, 2025
d5d09f8
remove new flags
tomtor Jul 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions clang/lib/Basic/Targets/AVR.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,10 @@ static MCUInfo AVRMcus[] = {
} // namespace targets
} // namespace clang

static bool ArchHasFLMAP(StringRef Name) {
return Name.starts_with("avr64") || Name.starts_with("avr128");
}

static bool ArchHasELPM(StringRef Arch) {
return llvm::StringSwitch<bool>(Arch)
.Cases("31", "51", "6", true)
Expand Down Expand Up @@ -529,6 +533,8 @@ void AVRTargetInfo::getTargetDefines(const LangOptions &Opts,
Builder.defineMacro("__AVR_ARCH__", Arch);

// TODO: perhaps we should use the information from AVRDevices.td instead?
if (ArchHasFLMAP(DefineName))
Builder.defineMacro("__AVR_HAVE_FLMAP__");
if (ArchHasELPM(Arch))
Builder.defineMacro("__AVR_HAVE_ELPM__");
if (ArchHasELPMX(Arch))
Expand Down
14 changes: 10 additions & 4 deletions llvm/lib/Target/AVR/AVRAsmPrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,11 +263,17 @@ bool AVRAsmPrinter::doFinalization(Module &M) {
auto *Section = cast<MCSectionELF>(TLOF.SectionForGlobal(&GO, TM));
if (Section->getName().starts_with(".data"))
NeedsCopyData = true;
else if (Section->getName().starts_with(".rodata") && SubTM->hasLPM())
else if (Section->getName().starts_with(".rodata") && SubTM->hasLPM()) {
// AVRs that have a separate program memory (that's most AVRs) store
// .rodata sections in RAM.
NeedsCopyData = true;
else if (Section->getName().starts_with(".bss"))
// .rodata sections in RAM,
// but XMEGA3 family maps all flash in the data space.
// Forcing pulling in __do_copy_data with 0 bytes to copy is a (minor)
// waste, so we let the loader handle this for newer devices.
if (!(SubTM->hasFeatureSetFamilyXMEGA2() ||
Copy link
Contributor

@Patryk27 Patryk27 Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we check for FeatureFLMAP in here instead?

Feels like that's what it is supposed to do, but we use family as a proxy for the actual flag we're interested in.

Copy link
Contributor Author

@tomtor tomtor Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comment, the new clang driver switch is just a feature, to select an option for FLMAP devices. It is not critical and could be removed from this PR. Only the code fragment you refer to is essential to fix the issue.

All other changes (mainly adding some metadata for newer xmega devices) just gets the clang feature set more in sync with gcc for newer AVR devices.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless trivial (e.g. typo fix), "marked this conversation as resolved." should only be used by reviewers per suggestions on https://discourse.llvm.org/t/rfc-github-pr-resolve-conversation-button/73178

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Sorry, didn't see that tomtor is the author :) )

SubTM->hasFeatureSetFamilyXMEGA3() ||
SubTM->hasFeatureSetFamilyXMEGA4()))
NeedsCopyData = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we check the rodata-in-ram switch here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, for these newer devices __do_copy_data should never be pulled in. This is done by the ld spec-files and crt for these devices. See also #146537

} else if (Section->getName().starts_with(".bss"))
NeedsClearBSS = true;
}

Expand Down
56 changes: 34 additions & 22 deletions llvm/lib/Target/AVR/AVRDevices.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,16 @@ def FeatureEIJMPCALL : SubtargetFeature<"eijmpcall", "HasEIJMPCALL", "true",
"The device supports the "
"`EIJMP`/`EICALL` instructions">;

// The device supports `FLMAP` flash bank data mapping to the data space.
def FeatureFLMAP : SubtargetFeature<"flmap", "HasFLMAP", "true",
"The device supports the "
"`FLMAP` mapping">;

// The device maps all flash (<= 32kB) to the data space.
def FeatureMAP : SubtargetFeature<"map", "HasMAP", "true",
"The device maps all flash (<= 32kB) "
"to the data space">;

// The device supports `ADDI Rd, K`, `SUBI Rd, K`.
def FeatureADDSUBIW : SubtargetFeature<"addsubiw", "HasADDSUBIW", "true",
"Enable 16-bit register-immediate "
Expand Down Expand Up @@ -220,6 +230,7 @@ def FamilyXMEGA3 : Family<"xmega3",
[FamilyAVR0, FeatureLPM, FeatureIJMPCALL,
FeatureADDSUBIW, FeatureSRAM, FeatureJMPCALL,
FeatureMultiplication, FeatureMOVW, FeatureLPMX,
FeatureMAP,
FeatureBREAK, FeatureLowByteFirst]>;

def FamilyXMEGA4 : Family<"xmega4",
Expand All @@ -228,6 +239,7 @@ def FamilyXMEGA4 : Family<"xmega4",
FeatureMultiplication, FeatureMOVW, FeatureLPMX,
FeatureELPM, FeatureELPMX,
FeatureSPM, FeatureSPMX,
FeatureFLMAP,
FeatureBREAK, FeatureLowByteFirst]>;

def FamilyXMEGA : Family<"xmega",
Expand Down Expand Up @@ -275,9 +287,9 @@ def : Device<"avr5", FamilyAVR5, ELFArchAVR5>;
def : Device<"avr51", FamilyAVR51, ELFArchAVR51>;
def : Device<"avr6", FamilyAVR6, ELFArchAVR6>;
def : Device<"avrxmega1", FamilyXMEGA, ELFArchXMEGA1>;
def : Device<"avrxmega2", FamilyXMEGA, ELFArchXMEGA2>;
def : Device<"avrxmega2", FamilyXMEGA2, ELFArchXMEGA2>;
def : Device<"avrxmega3", FamilyXMEGA3, ELFArchXMEGA3>;
def : Device<"avrxmega4", FamilyXMEGA, ELFArchXMEGA4>;
def : Device<"avrxmega4", FamilyXMEGA4, ELFArchXMEGA4>;
def : Device<"avrxmega5", FamilyXMEGA, ELFArchXMEGA5>;
def : Device<"avrxmega6", FamilyXMEGA, ELFArchXMEGA6>;
def : Device<"avrxmega7", FamilyXMEGA, ELFArchXMEGA7>;
Expand Down Expand Up @@ -596,26 +608,26 @@ def : Device<"atmega4809", FamilyXMEGA3, ELFArchXMEGA3>;

// Additions from gcc 14:

def : Device<"avr64da28", FamilyXMEGA2, ELFArchXMEGA2>;
def : Device<"avr64da32", FamilyXMEGA2, ELFArchXMEGA2>;
def : Device<"avr64da48", FamilyXMEGA2, ELFArchXMEGA2>;
def : Device<"avr64da64", FamilyXMEGA2, ELFArchXMEGA2>;
def : Device<"avr64db28", FamilyXMEGA2, ELFArchXMEGA2>;
def : Device<"avr64db32", FamilyXMEGA2, ELFArchXMEGA2>;
def : Device<"avr64db48", FamilyXMEGA2, ELFArchXMEGA2>;
def : Device<"avr64db64", FamilyXMEGA2, ELFArchXMEGA2>;
def : Device<"avr64dd14", FamilyXMEGA2, ELFArchXMEGA2>;
def : Device<"avr64dd20", FamilyXMEGA2, ELFArchXMEGA2>;
def : Device<"avr64dd28", FamilyXMEGA2, ELFArchXMEGA2>;
def : Device<"avr64dd32", FamilyXMEGA2, ELFArchXMEGA2>;
def : Device<"avr64du28", FamilyXMEGA2, ELFArchXMEGA2>;
def : Device<"avr64du32", FamilyXMEGA2, ELFArchXMEGA2>;
def : Device<"avr64ea28", FamilyXMEGA2, ELFArchXMEGA2>;
def : Device<"avr64ea32", FamilyXMEGA2, ELFArchXMEGA2>;
def : Device<"avr64ea48", FamilyXMEGA2, ELFArchXMEGA2>;
def : Device<"avr64sd28", FamilyXMEGA2, ELFArchXMEGA2>;
def : Device<"avr64sd32", FamilyXMEGA2, ELFArchXMEGA2>;
def : Device<"avr64sd48", FamilyXMEGA2, ELFArchXMEGA2>;
def : Device<"avr64da28", FamilyXMEGA2, ELFArchXMEGA2, [FeatureFLMAP]>;
def : Device<"avr64da32", FamilyXMEGA2, ELFArchXMEGA2, [FeatureFLMAP]>;
def : Device<"avr64da48", FamilyXMEGA2, ELFArchXMEGA2, [FeatureFLMAP]>;
def : Device<"avr64da64", FamilyXMEGA2, ELFArchXMEGA2, [FeatureFLMAP]>;
def : Device<"avr64db28", FamilyXMEGA2, ELFArchXMEGA2, [FeatureFLMAP]>;
def : Device<"avr64db32", FamilyXMEGA2, ELFArchXMEGA2, [FeatureFLMAP]>;
def : Device<"avr64db48", FamilyXMEGA2, ELFArchXMEGA2, [FeatureFLMAP]>;
def : Device<"avr64db64", FamilyXMEGA2, ELFArchXMEGA2, [FeatureFLMAP]>;
def : Device<"avr64dd14", FamilyXMEGA2, ELFArchXMEGA2, [FeatureFLMAP]>;
def : Device<"avr64dd20", FamilyXMEGA2, ELFArchXMEGA2, [FeatureFLMAP]>;
def : Device<"avr64dd28", FamilyXMEGA2, ELFArchXMEGA2, [FeatureFLMAP]>;
def : Device<"avr64dd32", FamilyXMEGA2, ELFArchXMEGA2, [FeatureFLMAP]>;
def : Device<"avr64du28", FamilyXMEGA2, ELFArchXMEGA2, [FeatureFLMAP]>;
def : Device<"avr64du32", FamilyXMEGA2, ELFArchXMEGA2, [FeatureFLMAP]>;
def : Device<"avr64ea28", FamilyXMEGA2, ELFArchXMEGA2, [FeatureFLMAP]>;
def : Device<"avr64ea32", FamilyXMEGA2, ELFArchXMEGA2, [FeatureFLMAP]>;
def : Device<"avr64ea48", FamilyXMEGA2, ELFArchXMEGA2, [FeatureFLMAP]>;
def : Device<"avr64sd28", FamilyXMEGA2, ELFArchXMEGA2, [FeatureFLMAP]>;
def : Device<"avr64sd32", FamilyXMEGA2, ELFArchXMEGA2, [FeatureFLMAP]>;
def : Device<"avr64sd48", FamilyXMEGA2, ELFArchXMEGA2, [FeatureFLMAP]>;

def : Device<"avr16dd20", FamilyXMEGA3, ELFArchXMEGA3>;
def : Device<"avr16dd28", FamilyXMEGA3, ELFArchXMEGA3>;
Expand Down
Loading