Page MenuHomePhabricator

[IndirectThunks] Make generated MF structure as expected by all instruction selectors.
ClosedPublic

Authored by kristof.beyls on Jun 8 2020, 8:15 AM.

Details

Summary

This also enables running the AArch64 SLSHardening pass with GlobalISel,
so add a test for that.

Diff Detail

Event Timeline

kristof.beyls created this revision.Jun 8 2020, 8:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2020, 8:15 AM
arsenm added a subscriber: arsenm.Jun 8 2020, 9:34 AM
arsenm added inline comments.
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
2496 ↗(On Diff #269244)

Why was this violated? Seems like it should be straightforward to fix? Can you add a pure IR test that just runs the IRTranslator?

kristof.beyls marked an inline comment as done.
kristof.beyls added inline comments.
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
2496 ↗(On Diff #269244)

The violation comes from thunks produced by the ThunkInserter. They are naked functions, not sure if that could trigger this issue.
I'll look into if it's possible to have a pure IR test trigger this.

kristof.beyls retitled this revision from Work around GlobalISel limitation on Indirect Thunks. to [IndirectThunks] Make generated MF structure as expected by all instruction selectors..
kristof.beyls edited the summary of this revision. (Show Details)
kristof.beyls added a reviewer: chandlerc.

This new revision fixes the root cause of GlobalISel asserting on an IndirectThunk: the IndirectThunk creation code was producing a MachineFunction with a slightly different structure than what gets produced from a naked function implemented in C code and processed by clang.
This fixes this by letting the IndirectThunk generator produce a MachineFunction structure that is the same as what gets produced by clang for an empty naked function written in C.

zbrid added a subscriber: zbrid.Jun 16 2020, 4:59 PM

LGTM

Could you explain how not adding the MBB entry block is different from removing extra MBB later? Curious on the details about the differences here since the change makes sense functionally and I don't see any issues, but I can't understand why adding the extra MBB entry block at the point we create the machine function and then deleting extra blocks results in something different than the automatically created entry block. Is it some attributes or configuration in the basic block or something?

Also slightly unrelated question: Thanks for moving the IndirectThunk code to be of use to more Targets earlier in the commit stack! Along those lines, I was wondering if you had additional thoughts recently on adding retpolines for AArch64 since you are working near the x86 retpoline code and there was an llvm-dev thread from a team member of mine a week or two ago?

llvm/include/llvm/CodeGen/IndirectThunks.h
68

nit: update this comment to only mention MachineFunctions since MBBs don't need to be created.

zbrid added a comment.Jun 16 2020, 5:01 PM
This comment was removed by zbrid.
zbrid accepted this revision.Jun 16 2020, 5:05 PM
zbrid added a reviewer: zbrid.
This revision is now accepted and ready to land.Jun 16 2020, 5:05 PM

LGTM

Could you explain how not adding the MBB entry block is different from removing extra MBB later? Curious on the details about the differences here since the change makes sense functionally and I don't see any issues, but I can't understand why adding the extra MBB entry block at the point we create the machine function and then deleting extra blocks results in something different than the automatically created entry block. Is it some attributes or configuration in the basic block or something?

This was triggering the following assert in GlobalISel:
https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp#L2495

The X86 target does not use globalisel by default, but the AArch64 target does use it by default at -O0. So now that IndirectThunks is used by AArch64 also, that has become a blocker.
Globalisel runs between the place where the MBB entry block was added and the block was removed later.
Apart from this assert blocking the compilation flow, I wouldn't expect code generation to be different.

Thanks for the explanation!

This revision was automatically updated to reflect the committed changes.
kristof.beyls marked 2 inline comments as done.Jun 18 2020, 12:00 AM
kristof.beyls added inline comments.
llvm/include/llvm/CodeGen/IndirectThunks.h
68

Thanks, I fixed that in a follow-up commit.