Page MenuHomePhabricator

[RISCV][MC] Find matching pcrel_hi fixup in more cases.
ClosedPublic

Authored by efriedma on Mar 4 2019, 5:42 PM.

Details

Summary

If a symbol points to the end of a fragment, instead of searching for fixups in that fragment, search in the next fragment.

Fixes spurious assembler error with subtarget change next to "la" pseudo-instruction, or expanded equivalent.

Alternate proposal to fix the problem discussed in https://reviews.llvm.org/D58759.

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma created this revision.Mar 4 2019, 5:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2019, 5:42 PM

On balance this patch seems a more appropriate solution given it doesn't touch generic code. I hope there isn't a case that isn't covered by this because I honestly can't think of one.

lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp
53 ↗(On Diff #189247)

Perhaps I'm missing something, but I don't quite follow why this change (AUIPCSRE->findAssociatedFragment() -> AUIPCSymbol->getFragment()) was made?

59 ↗(On Diff #189247)

Nitpick: could we swap this to Offset == DF->getContents().size()?

efriedma marked 2 inline comments as done.Mar 6 2019, 9:18 AM
efriedma added inline comments.
lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp
53 ↗(On Diff #189247)

It's a no-op. I just wanted to make the relationship between the fragment and the offset a bit more obvious.

59 ↗(On Diff #189247)

Sure.

apazos added a comment.Mar 6 2019, 6:32 PM

LGTM, I have verified the patch with some workloads and found no new issue.

asb accepted this revision.Mar 7 2019, 6:11 AM

It took me a bit of time to go through the alternate patches and related discussion, but I've now reviewed and this seems like the most straight-forward solution and the right way to go. Many thanks Eli. LGTM.

This revision is now accepted and ready to land.Mar 7 2019, 6:11 AM

Looking forward to seeing this patch merged. With this patch applied, I am nearly able to build FreeRTOS 10.2 with clang/RISC-V.

This revision was automatically updated to reflect the committed changes.