Page MenuHomePhabricator

[AArch64] Support stackmap/patchpoint in getInstSizeInBytes
ClosedPublic

Authored by rovka on Aug 31 2016, 5:06 AM.

Details

Summary

We currently return 4 for stackmaps and patchpoints, which is very optimistic
and can in rare cases cause the branch relaxation pass to fail to relax certain
branches.

This patch causes getInstSizeInBytes to return a pessimistic estimate of the
size as the number of bytes requested in the stackmap/patchpoint. In the future,
we could provide a more accurate estimate by sharing some of the logic in
AArch64::LowerSTACKMAP/PATCHPOINT.

Fixes part of https://llvm.org/bugs/show_bug.cgi?id=28750

Diff Detail

Repository
rL LLVM

Event Timeline

rovka updated this revision to Diff 69832.Aug 31 2016, 5:06 AM
rovka retitled this revision from to [AArch64] Support stackmap/patchpoint in getInstSizeInBytes.
rovka updated this object.
rovka added reviewers: t.p.northover, reames, kavon.
rovka added subscribers: llvm-commits, rengolin.
rengolin added inline comments.Aug 31 2016, 6:49 AM
lib/Target/AArch64/AArch64InstrInfo.cpp
64 ↗(On Diff #69832)

Nit: maybe make the switch set NumBytes and return it in the end?

The unreachable down there is silly, but probably silences a compiler warning. This way, we could get rid of that, too.

rovka updated this revision to Diff 69975.Sep 1 2016, 3:31 AM

Got rid of the unreachable, as Renato suggested.

Now that looks good to me.

I don't like much the tests, because if the branch relaxation algorithm change, this test can "fail" without really failing, and whoever hits this will have a hard time understanding how to make this test proper. But I have no better idea.

I'll leave this be for a while, see if anyone else has a better idea. If not, having those tests are better than nothing, and we can work on trying some unit tests in the future.

rovka added a comment.Sep 1 2016, 5:17 AM

I agree, unit tests would be a lot better, but I don't think we have a directory with AArch64-specific unit tests. We should try to add one at some point.

kavon edited edge metadata.Sep 10 2016, 1:58 PM

When the op code is a patchpoint, isn't a call still emitted? I think maybe the total number of bytes returned for patchpoints should include the bytes for the call.

rovka added a comment.Sep 12 2016, 1:19 AM

When the op code is a patchpoint, isn't a call still emitted? I think maybe the total number of bytes returned for patchpoints should include the bytes for the call.

From my reading of AArch64AsmPrinter::LowerPATCHPOINT, it looks like the number of bytes requested by the patchpoint has to include the bytes for the call - i.e. if the call is generated, its size is subtracted from the number of bytes requested, and only the remaining number of nops are added.

kavon accepted this revision.Sep 12 2016, 7:00 AM
kavon edited edge metadata.

When the op code is a patchpoint, isn't a call still emitted? I think maybe the total number of bytes returned for patchpoints should include the bytes for the call.

From my reading of AArch64AsmPrinter::LowerPATCHPOINT, it looks like the number of bytes requested by the patchpoint has to include the bytes for the call - i.e. if the call is generated, its size is subtracted from the number of bytes requested, and only the remaining number of nops are added.

Ah, okay. This looks good to me then. I'm a newcomer when it comes to contributing patches, so I'm not sure what else I should do. :)

This revision is now accepted and ready to land.Sep 12 2016, 7:00 AM

I'm a newcomer when it comes to contributing patches, so I'm not sure what else I should do. :)

You don't have to do anything else, I'll commit it tomorrow :) Thanks for reviewing.

This revision was automatically updated to reflect the committed changes.