This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Don't outline pcrel-lo operand.
ClosedPublic

Authored by kito-cheng on Aug 23 2022, 9:10 PM.

Details

Summary

This issue is found by build llvm-testsuite with -Oz, linker will complain
dangerous relocation: %pcrel_lo missing matching %pcrel_hi and that
turn out cause by we outlined pcrel-lo, but leave pcrel-hi there, that's
not problem in general, but the problem is they put into different section, they
pcrel-hi and pcrel-lo pair (e.g. AUIPC+ADDI) *MUST* put be present in same
section due to the implementation.

Outlined function will put into .text name, but the source functions
will put in .text.<function-name> if function-section is enabled or the
function has comdat attribute.

There are few solutions for this issue:

  1. Always disallow instructions with pcrel-lo flags.
  2. Only disallow instructions with pcrel-lo flags that when function-section is enabled or this function has comdat attribute.
  3. Check the corresponding instruction with pcrel-high also included in the outlining candidate sequence or not, and allow that only when pcrel-high is included in the outlining candidate.

First one is most conservative, that might lose some optimization
opportunities, and second one could save those opportunities, and last
one is hard to implement, and don't have any benefits since pcrel-high
are using different label even accessing same symbol.

Use custom section name might also cause this problem, but that already
filtered by RISCVInstrInfo::isFunctionSafeToOutlineFrom.

Diff Detail

Event Timeline

kito-cheng created this revision.Aug 23 2022, 9:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2022, 9:10 PM
kito-cheng requested review of this revision.Aug 23 2022, 9:10 PM
NOTE: This issue is appeared since https://reviews.llvm.org/D123265, pcrel-lo will propagated into other instructions after that.
pcwang-thead added a comment.EditedAug 23 2022, 9:33 PM

I met similar problem before when doing random testing via csmith, but I didn't have time to take a deep look. I think it looks great to me and thanks for the fix. :-)

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
1417

Nit: This line can be merged with line 1324, I think.

luismarques accepted this revision.Aug 24 2022, 6:08 AM

LGTM. Thanks Kito, I didn't realize that D123265 would have this interaction.

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
1422

Nit: "seperated" -> "separate". Also, "pcrel-high"-> "pcrel_hi", "pcrel-lo" -> "pcrel_lo".

This revision is now accepted and ready to land.Aug 24 2022, 6:08 AM
luismarques added inline comments.Aug 24 2022, 6:19 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
1422

I guess the "-" (vs "_") is fine, so it's just the "high" -> "hi".

This revision was landed with ongoing or failed builds.Aug 24 2022, 6:47 AM
This revision was automatically updated to reflect the committed changes.