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 2 commits into
base: main
Choose a base branch
from
+67 −28
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
2 commits
Select commit Hold shift + click to select a range
a08a6a0
[AVR] Handle mapped RO data for newer devices
tomtor Jun 27, 2025
d9f0f25
Fix comment
tomtor Jul 2, 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
4 changes: 4 additions & 0 deletions clang/include/clang/Driver/Options.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4886,6 +4886,10 @@ def mguard_EQ : Joined<["-"], "mguard=">, Group<m_Group>,
HelpText<"Enable or disable Control Flow Guard checks and guard tables emission">,
Values<"none,cf,cf-nochecks">;
def mmcu_EQ : Joined<["-"], "mmcu=">, Group<m_Group>;
def mflmap : Flag<["-"], "mflmap">, Group<m_Group>,
HelpText<"Use AVR mapped memory for RO data">;
def mrodata_in_ram : Flag<["-"], "mrodata-in-ram">, Group<m_Group>,
HelpText<"Copy RO data to ram">;
def msim : Flag<["-"], "msim">, Group<m_Group>;
def mfix_and_continue : Flag<["-"], "mfix-and-continue">, Group<clang_ignored_m_Group>;
def mieee_fp : Flag<["-"], "mieee-fp">, Group<clang_ignored_m_Group>;
Expand Down
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
15 changes: 13 additions & 2 deletions clang/lib/Driver/ToolChains/AVR.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,8 +651,19 @@ void AVR::Linker::ConstructJob(Compilation &C, const JobAction &JA,
// This is almost always required because otherwise avr-ld
// will assume 'avr2' and warn about the program being larger
// than the bare minimum supports.
if (Linker.find("avr-ld") != std::string::npos && FamilyName)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benshi001 If this line is relocated (moved a bit upwards) so that it is before handling user supplied linker arguments then a user could overwrite the default -m option.
As it is the user cannot overwrite the driver generated option.

CmdArgs.push_back(Args.MakeArgString(std::string("-m") + *FamilyName));
if (Linker.find("avr-ld") != std::string::npos && FamilyName) {
// Option to use mapped memory for modern devices with >32kB flash.
// This is the only option for modern devices with <= 32kB flash,
// but the larger default to a copy from flash to RAM (avr-ld version < 14)
// or map the highest 32kB to RAM (avr-ld version >= 14).
if (Args.hasFlag(options::OPT_mflmap, options::OPT_mrodata_in_ram, false)) {
CmdArgs.push_back(
Args.MakeArgString(std::string("-m") + *FamilyName + "_flmap"));
Copy link
Member

@MaskRay MaskRay Jul 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I am not familiar with these options. Are we sure this patch has listed all the effects?

gcc also seems to do something with the __do_copy_data symbol.

At the minimum, we need clang/test/Driver tests to check how a driver option influences the link options. For features, we need other tests (perhaps clang/test/CodeGen/AVR, which does not exist yet)

Copy link
Member

Choose a reason for hiding this comment

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

I agree, so I think I need more time to understand the whole picture what gcc did.

CmdArgs.push_back(Args.MakeArgString(std::string("-u")));
CmdArgs.push_back(Args.MakeArgString(std::string("__do_flmap_init")));
} else
CmdArgs.push_back(Args.MakeArgString(std::string("-m") + *FamilyName));
}

C.addCommand(std::make_unique<Command>(
JA, *this, ResponseFileSupport::AtFileCurCP(), Args.MakeArgString(Linker),
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