[PATCH] [AArch64] Fix problems in handling generic MSR/MRS instructions

[Petr Pavlu]

Thank you Renato for having a look at this patch.


REPOSITORY
  rL LLVM

================
Comment at: lib/Target/AArch64/Disassembler/AArch64Disassembler.cpp:1506
@@ +1505,3 @@
+  // If the name is not valid then the instruction must be threated as the
+  // extended MSR.
+  // FIXME This should be specified in TableGen but it is not currently
----------------
rengolin wrote:
> This seems like a gross exaggeration. Is it possible that the name would come invalid for any other reason?
I am not sure if I am really answering your question but `DecodeSystemPStateInstruction()` is called only for `MSRpstate` and this instruction aliases with an extended `MSR` format (introduced in r218753). All cases not allowed by `MSRpstate` are valid `MSR` cases.


Some more background on this code:

MC instruction `MSRpstate` is defined to have:
  Inst{31-19} = 0b1101010100000;
  Inst{18-16} = pstatefield{5-3};
  Inst{15-12} = 0b0100;
  Inst{11-8} = imm;
  Inst{7-5} = pstatefield{2-0};
  Inst{4-0} = 0b11111

A problem here is that there does not seem to be a good way how to specify that only a subset of `pstatefield` is allowed.

Disassembling a bitpattern that should be handled as an extended `MSR` (but which aliases `MSRpstate`):
  $ echo "0x1f 0x40 0x00 0xd5" | ./llvm-mc -triple=aarch64 -show-inst -show-encoding -disassemble -debug
  Args: ./llvm-mc -triple=aarch64 -show-inst -show-encoding -disassemble -debug 
          .text
  0: OPC_ExtractField(26, 3): 5
  3: OPC_FilterValue(2, 1110): FAIL: continuing at 1117
  1117: OPC_FilterValue(3, 28396): FAIL: continuing at 29517
  29517: OPC_FilterValue(4, 447): FAIL: continuing at 29968
  29968: OPC_FilterValue(5, 474): PASS: continuing at 29972
  29972: OPC_ExtractField(29, 3): 6
  29975: OPC_FilterValue(0, 4): FAIL: continuing at 29983
  29983: OPC_FilterValue(1, 39): FAIL: continuing at 30026
  30026: OPC_FilterValue(2, 17): FAIL: continuing at 30047
  30047: OPC_FilterValue(4, 4): FAIL: continuing at 30055
  30055: OPC_FilterValue(5, 39): FAIL: continuing at 30098
  30098: OPC_FilterValue(6, 10539): PASS: continuing at 30102
  30102: OPC_ExtractField(21, 5): 8
  30105: OPC_FilterValue(0, 30): FAIL: continuing at 30139
  30139: OPC_FilterValue(1, 10): FAIL: continuing at 30153
  30153: OPC_FilterValue(2, 11): FAIL: continuing at 30168
  30168: OPC_FilterValue(5, 30): FAIL: continuing at 30202
  30202: OPC_FilterValue(8, 122): PASS: continuing at 30206
  30206: OPC_ExtractField(0, 8): 31
  30209: OPC_FilterValue(95, 11): FAIL: continuing at 30224
  30224: OPC_FilterValue(159, 11): FAIL: continuing at 30240
  30240: OPC_FilterValue(191, 11): FAIL: continuing at 30256
  30256: OPC_FilterValue(223, 11): FAIL: continuing at 30272
  30272: OPC_ExtractField(0, 5): 31
  30275: OPC_FilterValue(31, 33): PASS: continuing at 30279
  30279: OPC_ExtractField(12, 4): 4
  30282: OPC_FilterValue(2, 11): FAIL: continuing at 30297
  30297: OPC_FilterValue(4, 11): PASS: continuing at 30301
  30301: OPC_CheckField(19, 1, 0, 5): FieldValue = 0, ExpectedValue = 0: PASS
  30307: OPC_Decode: opcode 1124, using decoder 216
  ----- DECODE SUCCESSFUL -----
  <stdin>:1:1: warning: invalid instruction encoding
  0x1f 0x40 0x00 0xd5
  ^

My understanding of the generated disassembler is that it works in two phases. In the first phase (before "DECODE SUCCESSFUL") the disassembler matches all constant bits and determines an MC opcode. In the second phase it calls a decode method specific to the determined opcode. In this case it calls explicit decode method `DecodeSystemPStateInstruction()`, it sees that `pstatefield` is not one of `SPSel`, `DAIFSet`, `DAIFClr` and rejects the instruction. No programmatic limitation on field values (without changing the resulting opcode) in the first phase, or a jump from the second phase back to the first one to handle the instruction as an extended `MSR` (after it is rejected as `MSRpstate`) seems possible.

I also considered an option to instantiate only the allowed `MSR` (immed) instructions (e.g. to have `MSRpstateSPSel`, `MSRpstateDAIFSet`, `MSRpstateDAIFClr`) but that does not look right because it creates a new MC opcode for each allowed `pstatefield` value.

A similar problem exists in the ARM backend where the preload hints fit into holes in the encodings of the `LDRH/STRH` instructions. It seems to be handled similarly in the instruction decode method.

http://reviews.llvm.org/D7174

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/