Page MenuHomePhabricator

[ARM] Turn some undefined encoding bits into 0s.
ClosedPublic

Authored by simon_tatham on Apr 15 2019, 5:59 AM.

Details

Summary

The family of 32-bit Thumb instruction encodings that include t2ORR,
t2AND and t2EOR are all listed in the ArmARM as having (0) in bit 15.
The Tablegen descriptions of those instructions listed them as ?. This
change tightens that up by making them into 0 + Unpredictable.

In the specific case of t2ORR, we tighten it up still further by
making the zero bit mandatory. This change comes from Arm v8.1-M, in
which encodings with that bit equal to 1 will now be used for
different instructions.

Diff Detail

Repository
rL LLVM

Event Timeline

simon_tatham created this revision.Apr 15 2019, 5:59 AM
This revision is now accepted and ready to land.Apr 15 2019, 10:06 AM
efriedma requested changes to this revision.Apr 18 2019, 3:19 PM
efriedma added a subscriber: efriedma.

The use of "?" here doesn't seem right. All those instructions where you're adding a "?" do in fact require that the bit in question must be zero.

We should also be using the "Unpredictable" field on these instructions so that we soft-fail in the disassembler. This shouldn't conflict with the v8.1-M instructions, I think? If it does, you can comment it out and leave a FIXME.

Please make sure we have tests in test/MC/Disassembler/ for all these encodings.

This revision now requires changes to proceed.Apr 18 2019, 3:19 PM

If I set the Unpredictable field on the ORR instruction in particular, then it clashes with CSEL. But I think you're right that all the other affected instructions can be changed from ? into 0 + Unpredictable.

Here's a revised version of the patch which does it that way, and also includes a test.

simon_tatham retitled this revision from [ARM] Turn some undefined encoding bits into mandatory 1s. to [ARM] Turn some undefined encoding bits into 0s..May 31 2019, 3:46 AM
simon_tatham edited the summary of this revision. (Show Details)
efriedma added inline comments.May 31 2019, 12:14 PM
llvm/lib/Target/ARM/ARMInstrThumb2.td
895 ↗(On Diff #202410)

Can you use Unpredictable here?

1889 ↗(On Diff #202410)

Can you use Unpredictable here?

simon_tatham marked 2 inline comments as done.Jun 3 2019, 1:47 AM
simon_tatham added inline comments.
llvm/lib/Target/ARM/ARMInstrThumb2.td
895 ↗(On Diff #202410)

No – this encoding is a subset of the ORR encoding, which is the one that v8.1-M has tightened up to make the 0 bit mandatory. If I turn this into a softfail, then the v8.1-M conditional select instructions added by D62667 will be mis-decoded as unpredictable variants of move or shift instructions.

1889 ↗(On Diff #202410)

No – same reason as above.

efriedma accepted this revision.Jun 3 2019, 3:23 PM

LGTM

llvm/lib/Target/ARM/ARMInstrThumb2.td
895 ↗(On Diff #202410)

Oh, I didn't realize the encodings worked that way. Makes sense.

This revision is now accepted and ready to land.Jun 3 2019, 3:23 PM
Closed by commit rL362470: [ARM] Turn some undefined encoding bits into 0s. (authored by statham, committed by ). · Explain WhyJun 4 2019, 1:25 AM
This revision was automatically updated to reflect the committed changes.