Page MenuHomePhabricator

[AArch64] Emit the correct MCExpr relocations specifiers like VK_ABS_G0, etc
ClosedPublic

Authored by mgrang on Dec 21 2018, 4:32 PM.

Details

Summary

D55896 and D56029 add support to emit fixups for :abs_g0: , :abs_g1_s: , etc.
This patch adds the necessary enums and MCExpr needed for lowering these.

Diff Detail

Repository
rL LLVM

Event Timeline

mgrang created this revision.Dec 21 2018, 4:32 PM

I am not sure what kind of tests I should add for this patch. This is one of the patches needed for adding SEH support to LLVM (D53540).

You can write an MIR test, probably; check the assembly that gets emitted for an instruction like like "renamable $x0 = MOVKXi killed renamable $x0, target-flags(aarch64-g1, aarch64-nc) @a, 16".

Also, you probably need to update AArch64InstrInfo::getSerializableBitmaskMachineOperandTargetFlags .

mgrang updated this revision to Diff 179518.Dec 26 2018, 1:06 PM
rnk added a comment.Jan 3 2019, 6:52 PM

Still needs tests, right?

In D56037#1346032, @rnk wrote:

Still needs tests, right?

Yes, I will add tests. Also I see that AArch64MCExpr::VK_NC is not set when AArch64II::MO_NC is set. Will fix this soon.

mgrang marked an inline comment as done.Jan 4 2019, 11:39 AM
mgrang added inline comments.
lib/Target/AArch64/AArch64MCInstLower.cpp
212 ↗(On Diff #179518)

The problem with setting VK_NC (if MO_NC is set) is that a lot of unit tests fail with the following assert:

        adrp    x0, "??_C@foo@"
        add     x0, x0, Invalid ELF symbol kind
UNREACHABLE executed at /local/mnt/workspace/mgrang/comm/src/llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCExpr.cpp:78!

This is because a lot of flags like VK_PAGEOFF/VK_PAGE, etc also have to be set (like LowerSymbolELF does it). I am not sure that should be done in this patch.

Should I then restrict setting VK_NC only if one of MO_G3/MO_G2/MO_G1/MO_G0 and MO_NC are set?

mgrang updated this revision to Diff 180295.Jan 4 2019, 12:10 PM
mgrang updated this revision to Diff 180518.Jan 7 2019, 10:23 AM
efriedma added inline comments.Jan 7 2019, 11:32 AM
lib/Target/AArch64/AArch64MCInstLower.cpp
233 ↗(On Diff #180518)

You can declare RefKind with type "auto" since it's obvious here.

212 ↗(On Diff #179518)

Ultimately, we should only form an AArch64MCExprs where getKind() returns a known kind (one of the enum values from the list in AArch64MCExpr); otherwise, it's not clear what relocation is being requested.

I'm fine guarding it with "if (RefFlags)" or something like that for now (so we VK_NONE for cases you aren't addressing with this patch). Please leave a FIXME; we should clean it up at some point for clarity, but it doesn't need to block this patch.

mgrang updated this revision to Diff 180979.Jan 9 2019, 6:33 PM
mgrang updated this revision to Diff 180980.
efriedma added inline comments.Jan 9 2019, 6:44 PM
lib/Target/AArch64/AArch64MCInstLower.cpp
203 ↗(On Diff #180980)

Formatting

mgrang updated this revision to Diff 180982.Jan 9 2019, 6:51 PM
mgrang marked an inline comment as done.
efriedma accepted this revision.Jan 9 2019, 6:53 PM

LGTM

This revision is now accepted and ready to land.Jan 9 2019, 6:53 PM
This revision was automatically updated to reflect the committed changes.