This is an archive of the discontinued LLVM Phabricator instance.

ARM: Improve handling of the Thumb2 M-class MSR instruction
ClosedPublic

Authored by petpav01 on Aug 20 2014, 12:39 AM.

Details

Reviewers
rengolin
Summary

Hi,

This patch implements a few changes related to the Thumb2 M-class MSR instruction:

  • better handling of unpredictable encodings,
  • recognition of the _g and _nzcvqg variants by the asm parser only if the DSP extension is available,
  • preferred output of MSR APSR moves with the _<bits> suffix for v7-M.

Thanks,
Petr

Diff Detail

Event Timeline

petpav01 updated this revision to Diff 12690.Aug 20 2014, 12:39 AM
petpav01 retitled this revision from to ARM: Improve handling of the Thumb2 M-class MSR instruction.
petpav01 updated this object.
petpav01 edited the test plan for this revision. (Show Details)
petpav01 added a subscriber: Unknown Object (MLST).

Hi Petr,

Some comments inline, but otherwise looks good.

Thanks,
--renato

lib/Target/ARM/ARMInstrThumb2.td
4023–4026

I know it was there before, but this "mask" could also be called "SYSm" for consistency

4025

Isn't this the same as saying:

let Unpredictable{20} = 0b1
lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp
817

Was the t2MRS_M a typo? I'm surprised it worked at all...

rengolin removed a subscriber: rengolin.

Hi Renato,

Thank you for looking at this patch. Responses to your comments inlined.

Petr

lib/Target/ARM/ARMInstrThumb2.td
4023–4026

Makes sense. I will attach a new patch. I also noticed that line "let Inst{19-16} = 0b1111;" is not necessary because the information is already provided in "let Inst{31-12} = 0b11110011111011111000;" so the new patch removes it.

4025

It does not seem to be same. With "let Unpredictable{20-16} = 0b11111;":

$ ./bin/llvm-mc --disassemble -triple=thumbv7em
0xe0 0xf3 0x00 0x80
        .text
<stdin>:1:1: warning: potentially undefined instruction encoding
0xe0 0xf3 0x00 0x80
^
        mrs     r0, apsr

With "let Unpredictable{20} = 0b1;":

$ ./bin/llvm-mc --disassemble -triple=thumbv7em
0xe0 0xf3 0x00 0x80
        .text
<stdin>:1:1: warning: invalid instruction encoding
0xe0 0xf3 0x00 0x80
^
        lsls    r3, r6, #3
<stdin>:1:16: warning: invalid instruction encoding
0xe0 0xf3 0x00 0x80
               ^

The first output is the expected one.

lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp
817

It was not a typo. The patch structures the code in a way that this condition is now inverted (MRS -> MSR).

petpav01 updated this revision to Diff 13076.Aug 29 2014, 3:53 AM
rengolin accepted this revision.Aug 30 2014, 3:56 AM
rengolin edited edge metadata.

Right, LGTM, thanks!

--renato

This revision is now accepted and ready to land.Aug 30 2014, 3:56 AM

Hi Renato,

Thank your for the review. Could you please commit the patch for me? I do not
have the commit rights.

Thanks,
Petr

rengolin closed this revision.Sep 1 2014, 4:34 AM

Committed in r216874.

Thanks,
--renato