Page MenuHomePhabricator

[PowerPC] Allow absolute expressions in relocations
ClosedPublic

Authored by nemanjai on Dec 8 2021, 7:59 PM.

Details

Reviewers
nickdesaulniers
Group Reviewers
Restricted Project
Commits
rG2aaba44b5c22: [PowerPC] Allow absolute expressions in relocations
Summary

The Linux kernel build uses absolute expressions suffixed with @lo/@ha relocations. This currently doesn't work for DS/DQ form instructions and there is no reason for it not to. It also works with GAS.
This patch allows this as long as the value is a multiple of 4/16 for DS/DQ form.

Diff Detail

Event Timeline

nemanjai created this revision.Dec 8 2021, 7:59 PM
nemanjai requested review of this revision.Dec 8 2021, 7:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2021, 7:59 PM

Thanks for the patch!

llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp
345–367

These two functions look very similar except for one constant. Consider calling a new helper that shares an implementation but accepts the constant as a parameter.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCExpr.cpp
114

Using Fixup->getTargetKind() should avoid the need for the casts.

llvm/test/MC/PowerPC/ppc64-abs-reloc.s
4–8

I'm guessing these directives and the cfi directives below are unnecessary to the functionality of this test? I'd be curious if this test case could be pared down further?

nemanjai added inline comments.Jan 14 2022, 2:44 PM
llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp
345–367

Sure, but I don't really think there's a way to pass parameters to it from the .td description (see lines 1018 and 1032 of PPCInstrInfo.td). I will turn them into wrappers for a function that does the computation.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCExpr.cpp
114

Sounds good.

llvm/test/MC/PowerPC/ppc64-abs-reloc.s
4–8

I just left what I got from the back end when compiling. I agree that they are not needed and will remove them.

nemanjai updated this revision to Diff 400168.Jan 14 2022, 3:36 PM

Fold the logic for immediate operands into one function. Simplify the test case and remove some unnecessary casts.

llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp
693

the ternary checking Signed will always be true in this branch of the if (Signed) one line above it.

696–697

and this ternary will always be false.

698

should the unsigned branch be calling getImmU16Context instead of getImmS16Context?

Hmm... for whatever reason, I forgot that these checks are in signed/unsigned blocks already. Update coming up.

nemanjai updated this revision to Diff 400187.Jan 14 2022, 4:25 PM

Fix the queries for the signed/unsigned cases.

Maybe I am misunderstanding the relocation fixups and DQ form instructions, if so, please ignore my comments. : )

llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp
47

For DQ form the adjusted value should be Value & 0xfff0? The last 4 bits are 0?

llvm/lib/Target/PowerPC/MCTargetDesc/PPCFixupKinds.h
54

For DQ form, the fixup(DQ field?) should be 12 bits and 4 implied zero bits?

Maybe I am misunderstanding the relocation fixups and DQ form instructions, if so, please ignore my comments. : )

For details of how this stuff works (i.e. constraints on relocations) please see section 3.5.1 Relocation Fields of the ELF V2 ABI Specification.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp
47

No, these two are the exact same thing, we just need to differentiate them by name. This is only to differentiate the two when we need to compute and validate the relocations (see the code in PPCMCExpr.cpp).

llvm/lib/Target/PowerPC/MCTargetDesc/PPCFixupKinds.h
54

Oh, for the fixup, it should just be 2 bits (i.e. this is the exact same thing as the ds fixup).

nickdesaulniers accepted this revision.Jan 24 2022, 11:23 AM

Thanks @nemanjai . Consider cleaning up some whitespace nits in the newly added test before comitting.

llvm/test/MC/PowerPC/ppc64-abs-reloc.s
3

remove extra whitespace between -D and -r

4–16

The indentation here looks like mixed usage of spaces and tabs. Let's pick one, and be consistent.

This revision is now accepted and ready to land.Jan 24 2022, 11:23 AM
This revision was landed with ongoing or failed builds.Feb 22 2022, 7:53 AM
This revision was automatically updated to reflect the committed changes.