-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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.
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
[Driver] Default enable LoongArch linker relaxation #111488
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -134,6 +134,13 @@ void loongarch::getLoongArchTargetFeatures(const Driver &D, | |
(!Args.hasArgNoClaim(clang::driver::options::OPT_march_EQ))) | ||
Features.push_back("+lsx"); | ||
|
||
// -mrelax is default, unless -mno-relax is specified. | ||
if (Args.hasFlag(options::OPT_mrelax, options::OPT_mno_relax, true)) { | ||
Features.push_back("+relax"); | ||
} else { | ||
Features.push_back("-relax"); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, it reminds me to check out llvm's codestandard. |
||
std::string ArchName; | ||
if (const Arg *A = Args.getLastArg(options::OPT_march_EQ)) | ||
ArchName = A->getValue(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1462,6 +1462,8 @@ template <class ELFT> void Writer<ELFT>::finalizeAddressDependentContent() { | |
for (;;) { | ||
bool changed = ctx.target->needsThunks | ||
? tc.createThunks(pass, ctx.outputSections) | ||
: ctx.arg.emachine == EM_LOONGARCH && !ctx.arg.relax | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
ctx.arg.relax is used here because we want to determine whether or not to apply relaxation based on the parameters passed at the time of linking. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think so. R_LARCH_ALIGN should be handled unconditionally, otherwise the code may be unaligned. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, it was an oversight. So for scenarios outside of align, is it necessary to determine whether or not to apply relaxation via the incoming link parameter, i.e. --relax? For example, in the relax function There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm afraid it's not necessary because R_LARCH_RELAX is converted to R_NONE by function
Is it correct? @MQ-mengqing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Thanks for the reminder, I tested both on RISCV and LoongArch, the relocation which corresponding RelExpr is R_NONE will be ignored. The changes of test files will be reverted. |
||
? false | ||
: ctx.target->relaxOnce(pass); | ||
bool spilled = ctx.script->spillSections(); | ||
changed |= spilled; | ||
|
@@ -1545,7 +1547,8 @@ template <class ELFT> void Writer<ELFT>::finalizeAddressDependentContent() { | |
finalizeOrderDependentContent(); | ||
} | ||
} | ||
if (!ctx.arg.relocatable) | ||
if (!ctx.arg.relocatable && | ||
!(ctx.arg.emachine == EM_LOONGARCH && !ctx.arg.relax)) | ||
ctx.target->finalizeRelax(pass); | ||
|
||
if (ctx.arg.relocatable) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code self explains and the comment is not necessary.