[MC layer][AArch64] llvm-mc accepts 4-bit immediate values for "msr pan, #imm"

[labrinea]

Summary

llvm-mc incorrectly encodes and decodes "msr pan, #imm" instructions. Only 1-bit immediate values should be valid. Currently 4-bit immediates are accepted.

echo "0x9f 0x42 0x00 0xd5" | llvm-mc -disassemble -triple=aarch64 -mattr=+v8.1a
        .text
        msr     PAN, #2
$ echo "msr pan, #2" | llvm-mc -triple=aarch64 -mattr=+v8.1a
        .text
        msr     PAN, #2

[rengolin]

Hi Alexandros,

Is this really an error? The MSR immediate is a 4-bit field. I understand why privileged access only needs a boolean, but if the CPU ignores the other three bits, we could maybe safely ignore it? Just a guess....

cheers,
--renato

[labrinea]

As you can see from the above commands, the MC layer does not give any errors or warnings when encoding an assembly string. Furthermore it incorrectly decodes a byte stream.

[rengolin]

In D13011#249979, @labrinea wrote:

As you can see from the above commands, the MC layer does not give any errors or warnings when encoding an assembly string. Furthermore it incorrectly decodes a byte stream.

No, I mean, in the CPU. Would that trigger a trap if the immediate is not 000x?

[grosbach]

The MSR instruction is explicitly documented as taking a 4-bit immediate operand.

You're arguing that the assembler should contextually decide what is or isn't legal for the second operand based on the value of the first? That's generally not something we do for these sorts of instructions unless there is documentation requiring it.

[rengolin]

In D13011#249988, @grosbach wrote:

You're arguing that the assembler should contextually decide what is or isn't legal for the second operand based on the value of the first? That's generally not something we do for these sorts of instructions unless there is documentation requiring it.

I second that. Until we have an update on the ARM ARM stating that PAM's immediate has to be 1 bit encoded and/or we have hardware to show that it traps, we shouldn't be changing this.

[olista01]

Until we have an update on the ARM ARM stating that PAM's immediate has to be 1 bit encoded

The ARM ARMv8.1 (which hasn't been published yet, but LLVM's implementation of ARMv8.1 is based on it) lists all encodings of "MSR PAN" with immediates > 1 as UNPREDICTABLE, meaning that the processor may do any of these:

• Trap
• Treat the instruction as a NOP
• Ignore the high bits of the immediate
• Write an arbitrary value to the destination register

[labrinea]

Shall we reconsider keeping this patch after Oliver's feedback?

[rengolin]

In D13011#254532, @olista01 wrote:

The ARM ARMv8.1 (which hasn't been published yet, but LLVM's implementation of ARMv8.1 is based on it) lists all encodings of "MSR PAN" with immediates > 1 as UNPREDICTABLE

When is it going to be public?

I'm not doubting you, but I don't want to set the precedent to implement things that aren't public. Though, as you say, all ARMv8.1 is based on that document, so I'm not sure what to choose.

Personally, I'm ok with this going in, as long as we're explicit that this is v8.1 only (even if PAN is not available before, not everyone knows that). Jim, are you ok with this, too?

Also, please pass clang-format on your change.

cheers,
--renato

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

3929 ↗

(On Diff #35231)

Include the fact that this is armv8.1 only in the comment.

3946 ↗

(On Diff #35231)

bad format, please pass clang-format on this.

[grosbach]

I don't see the text which describes the other values as UNPREDICTABLE. Can you provide a doc number and section reference? The v8.1 extension doc describes the other bits of the CRm field as being zero, but I don't see a mention of what happens if they're not. I believe you, I'm just wondering where I should be looking that I'm obviously not.

That aside, is there perhaps a cleaner way to implement this? Perhaps via a generic instruction definition that allows all encoding values and then assembly aliases that have more restricted operand sets? If we can avoid it, I'd really like to stay away from more C++ fiddling in the parser. We already have way too much of it. :(

[olista01]

Can you provide a doc number and section reference?

The v8.1 spec, section 8.1.3, lists the top 3 bits of the CRm field as "(0)(0)(0)". The v8A ARMARM, section C2.2.1 defines that the behaviour is CONSTRAINED UNPREDICTABLE if any of these bits are 1, and lists the allowed behaviours.

[labrinea]

Reimplemented via a generic instruction definition that allows all encoding values and then assembly aliases that have more restricted operand sets.

[rengolin]

Hi Alexandros,

With the nitpick, looks good to me. Thanks!

lib/Target/AArch64/AArch64ISelDAGToDAG.cpp

2268 ↗

(On Diff #36340)

Nitpick: I'd extract this on the line above, like:

auto State = Immed < 2 ? AArch64::MSRpstateImm1 : AArch64::MSRpstateImm4;
return CurDAG->getMachineNode(State, DL, ...

[labrinea]

lib/Target/AArch64/AArch64ISelDAGToDAG.cpp

2268 ↗

(On Diff #36340)

Sure, thanks!

[rengolin]

lib/Target/AArch64/AArch64ISelDAGToDAG.cpp

2268 ↗

(On Diff #36340)

Wait, wouldn't this make it use the wrong imm encoding for other calls if their values happened to be < 2?

[labrinea]

lib/Target/AArch64/AArch64ISelDAGToDAG.cpp

2268 ↗

(On Diff #36340)

Good point the check should be based on the value of Reg, and MVT::i16 should be changed to MVT::i1 too for AArch64::MSRpstateImm1 opcode. I am updating the patch.

[labrinea]

lib/Target/AArch64/AArch64ISelDAGToDAG.cpp

2268 ↗

(On Diff #36340)

Sorry I got confused, I thought MVT::i16 means values from 0 to 16 rather than 16 bits. I think MVT enumeration doesn't really matter since MVT::i16 was already more than the 4 bits required for the Immediate. I think we should just change the patch like this:

auto State = (Reg == AArch64PState::PAN) ? AArch64::MSRpstateImm1
                                         : AArch64::MSRpstateImm4;

[rengolin]

lib/Target/AArch64/AArch64ISelDAGToDAG.cpp

2268 ↗

(On Diff #36340)

Yes, that's what I was thinking, too. Maybe also adding an assert?

if (Reg == AArch64PState::PAN)
  assert(Immed < 2 && "Bad imm");

[labrinea]

AArch64DAGToDAGISel::SelectWriteRegister updated.

[rengolin]

With the nitpick, LGTM. Thanks!

lib/Target/AArch64/AArch64ISelDAGToDAG.cpp

2272 ↗

(On Diff #36499)

nitpick: no new line here. :)

llvm/trunk/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp

SDNode *AArch64DAGToDAGISel::SelectWriteRegister(SDNode *N) {
   bool IsValidSpecialReg;
   Reg = PMapper.fromString(RegString->getString(),
                            Subtarget->getFeatureBits(),
                            IsValidSpecialReg);
   if (IsValidSpecialReg) {
     assert (isa<ConstantSDNode>(N->getOperand(2))
               && "Expected a constant integer expression.");
     uint64_t Immed = cast<ConstantSDNode>(N->getOperand(2))->getZExtValue();
-    return CurDAG->getMachineNode(AArch64::MSRpstate, DL, MVT::Other,
+    unsigned State;
+    if (Reg == AArch64PState::PAN) {
+      assert(Immed < 2 && "Bad imm");
+      State = AArch64::MSRpstateImm1;
+    } else {
+      assert(Immed < 16 && "Bad imm");
+      State = AArch64::MSRpstateImm4;
+    }
+    return CurDAG->getMachineNode(State, DL, MVT::Other,
                                   CurDAG->getTargetConstant(Reg, DL, MVT::i32),
                                   CurDAG->getTargetConstant(Immed, DL, MVT::i16),
                                   N->getOperand(0));
   }
 
   // Use the sysreg mapper to attempt to map the remaining possible strings
   // to the value for the register to be used for the MSR (register)
   // instruction operand.

llvm/trunk/lib/Target/AArch64/AArch64InstrFormats.td

 def vecshiftR64Narrow : Operand<i32>, ImmLeaf<i32, [{
   return (((uint32_t)Imm) > 0) && (((uint32_t)Imm) < 33);
 }]> {
   let EncoderMethod = "getVecShiftR64OpValue";
   let DecoderMethod = "DecodeVecShiftR64ImmNarrow";
   let ParserMatchClass = Imm1_32Operand;
 }
 
+def Imm0_1Operand : AsmImmRange<0, 1>;
 def Imm0_7Operand : AsmImmRange<0, 7>;
 def Imm0_15Operand : AsmImmRange<0, 15>;
 def Imm0_31Operand : AsmImmRange<0, 31>;
 def Imm0_63Operand : AsmImmRange<0, 63>;
 
 def vecshiftL8 : Operand<i32>, ImmLeaf<i32, [{
   return (((uint32_t)Imm) < 8);
 }]> {

 
 // True if the 32-bit immediate is in the range [0,31]
 def imm32_0_31 : Operand<i32>, ImmLeaf<i32, [{
   return ((uint64_t)Imm) < 32;
 }]> {
   let ParserMatchClass = Imm0_31Operand;
 }
 
+// imm0_1 predicate - True if the immediate is in the range [0,1]
+def imm0_1 : Operand<i64>, ImmLeaf<i64, [{
+  return ((uint64_t)Imm) < 2;
+}]> {
+  let ParserMatchClass = Imm0_1Operand;
+}
+
 // imm0_15 predicate - True if the immediate is in the range [0,15]
 def imm0_15 : Operand<i64>, ImmLeaf<i64, [{
   return ((uint64_t)Imm) < 16;
 }]> {
   let ParserMatchClass = Imm0_15Operand;
 }
 
 // imm0_7 predicate - True if the immediate is in the range [0,7]

 // Explicitly modeling each of the system register as a register class
 // would do it, but feels like overkill at this point.
 class MSRI : RtSystemI<0, (outs), (ins msr_sysreg_op:$systemreg, GPR64:$Rt),
                        "msr", "\t$systemreg, $Rt"> {
   bits<16> systemreg;
   let Inst{20-5} = systemreg;
 }
 
-def SystemPStateFieldOperand : AsmOperandClass {
-  let Name = "SystemPStateField";
+def SystemPStateFieldWithImm0_15Operand : AsmOperandClass {
+  let Name = "SystemPStateFieldWithImm0_15";
   let ParserMethod = "tryParseSysReg";
 }
-def pstatefield_op : Operand<i32> {
-  let ParserMatchClass = SystemPStateFieldOperand;
+def pstatefield4_op : Operand<i32> {
+  let ParserMatchClass = SystemPStateFieldWithImm0_15Operand;
   let PrintMethod = "printSystemPStateField";
 }
 
 let Defs = [NZCV] in
-class MSRpstateI
-  : SimpleSystemI<0, (ins pstatefield_op:$pstate_field, imm0_15:$imm),
-                  "msr", "\t$pstate_field, $imm">,
+class MSRpstateImm0_15
+  : SimpleSystemI<0, (ins pstatefield4_op:$pstatefield, imm0_15:$imm),
+                  "msr", "\t$pstatefield, $imm">,
     Sched<[WriteSys]> {
   bits<6> pstatefield;
   bits<4> imm;
   let Inst{20-19} = 0b00;
   let Inst{18-16} = pstatefield{5-3};
   let Inst{15-12} = 0b0100;
   let Inst{11-8} = imm;
   let Inst{7-5} = pstatefield{2-0};
 
   let DecoderMethod = "DecodeSystemPStateInstruction";
   // MSRpstateI aliases with MSRI. When the MSRpstateI decoder method returns
   // Fail the decoder should attempt to decode the instruction as MSRI.
   let hasCompleteDecoder = 0;
 }
 
+def SystemPStateFieldWithImm0_1Operand : AsmOperandClass {
+  let Name = "SystemPStateFieldWithImm0_1";
+  let ParserMethod = "tryParseSysReg";
+}
+def pstatefield1_op : Operand<i32> {
+  let ParserMatchClass = SystemPStateFieldWithImm0_1Operand;
+  let PrintMethod = "printSystemPStateField";
+}
+
+let Defs = [NZCV] in
+class MSRpstateImm0_1
+  : SimpleSystemI<0, (ins pstatefield1_op:$pstatefield, imm0_1:$imm),
+                  "msr", "\t$pstatefield, $imm">,
+    Sched<[WriteSys]> {
+  bits<6> pstatefield;
+  bit imm;
+  let Inst{20-19} = 0b00;
+  let Inst{18-16} = pstatefield{5-3};
+  let Inst{15-9} = 0b0100000;
+  let Inst{8} = imm;
+  let Inst{7-5} = pstatefield{2-0};
+
+  let DecoderMethod = "DecodeSystemPStateInstruction";
+  // MSRpstateI aliases with MSRI. When the MSRpstateI decoder method returns
+  // Fail the decoder should attempt to decode the instruction as MSRI.
+  let hasCompleteDecoder = 0;
+}
+
 // SYS and SYSL generic system instructions.
 def SysCRAsmOperand : AsmOperandClass {
   let Name = "SysCR";
   let ParserMethod = "tryParseSysCROperand";
 }
 
 def sys_cr_op : Operand<i32> {
   let PrintMethod = "printSysCROperand";

llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.td

def ISB   : CRmSystemI<barrier_op, 0b110, "isb",
                        [(int_aarch64_isb (i32 imm32_0_15:$CRm))]>;
 }
 
 def : InstAlias<"clrex", (CLREX 0xf)>;
 def : InstAlias<"isb", (ISB 0xf)>;
 
 def MRS    : MRSI;
 def MSR    : MSRI;
-def MSRpstate: MSRpstateI;
+def MSRpstateImm1 : MSRpstateImm0_1;
+def MSRpstateImm4 : MSRpstateImm0_15;
 
 // The thread pointer (on Linux, at least, where this has been implemented) is
 // TPIDR_EL0.
 def : Pat<(AArch64threadpointer), (MRS 0xde82)>;
 
 // The cycle counter PMC register is PMCCNTR_EL0.
 let Predicates = [HasPerfMon] in
 def : Pat<(readcyclecounter), (MRS 0xdce8)>;

llvm/trunk/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

template <int Scale> bool isUImm12Offset() const {
     const MCConstantExpr *MCE = dyn_cast<MCConstantExpr>(getImm());
     if (!MCE)
       return isSymbolicUImm12Offset(getImm(), Scale);
 
     int64_t Val = MCE->getValue();
     return (Val % Scale) == 0 && Val >= 0 && (Val / Scale) < 0x1000;
   }
 
+  bool isImm0_1() const {
+    if (!isImm())
+      return false;
+    const MCConstantExpr *MCE = dyn_cast<MCConstantExpr>(getImm());
+    if (!MCE)
+      return false;
+    int64_t Val = MCE->getValue();
+    return (Val >= 0 && Val < 2);
+  }
   bool isImm0_7() const {
     if (!isImm())
       return false;
     const MCConstantExpr *MCE = dyn_cast<MCConstantExpr>(getImm());
     if (!MCE)
       return false;
     int64_t Val = MCE->getValue();
     return (Val >= 0 && Val < 8);
public:
   bool isSysReg() const { return Kind == k_SysReg; }
   bool isMRSSystemRegister() const {
     if (!isSysReg()) return false;
 
     return SysReg.MRSReg != -1U;
   }
   bool isMSRSystemRegister() const {
     if (!isSysReg()) return false;
-
     return SysReg.MSRReg != -1U;
   }
-  bool isSystemPStateField() const {
+  bool isSystemPStateFieldWithImm0_1() const {
     if (!isSysReg()) return false;
-
+    return SysReg.PStateField == AArch64PState::PAN;
+  }
+  bool isSystemPStateFieldWithImm0_15() const {
+    if (!isSysReg() || isSystemPStateFieldWithImm0_1()) return false;
     return SysReg.PStateField != -1U;
   }
   bool isReg() const override { return Kind == k_Register && !Reg.isVector; }
   bool isVectorReg() const { return Kind == k_Register && Reg.isVector; }
   bool isVectorRegLo() const {
     return Kind == k_Register && Reg.isVector &&
            AArch64MCRegisterClasses[AArch64::FPR128_loRegClassID].contains(
                Reg.RegNum);

   }
 
   void addSImm7s16Operands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     const MCConstantExpr *MCE = cast<MCConstantExpr>(getImm());
     Inst.addOperand(MCOperand::createImm(MCE->getValue() / 16));
   }
 
+  void addImm0_1Operands(MCInst &Inst, unsigned N) const {
+    assert(N == 1 && "Invalid number of operands!");
+    const MCConstantExpr *MCE = cast<MCConstantExpr>(getImm());
+    Inst.addOperand(MCOperand::createImm(MCE->getValue()));
+  }
+
   void addImm0_7Operands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     const MCConstantExpr *MCE = cast<MCConstantExpr>(getImm());
     Inst.addOperand(MCOperand::createImm(MCE->getValue()));
   }
 
   void addImm1_8Operands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");

   }
 
   void addMSRSystemRegisterOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
 
     Inst.addOperand(MCOperand::createImm(SysReg.MSRReg));
   }
 
-  void addSystemPStateFieldOperands(MCInst &Inst, unsigned N) const {
+  void addSystemPStateFieldWithImm0_1Operands(MCInst &Inst, unsigned N) const {
+    assert(N == 1 && "Invalid number of operands!");
+
+    Inst.addOperand(MCOperand::createImm(SysReg.PStateField));
+  }
+
+  void addSystemPStateFieldWithImm0_15Operands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
 
     Inst.addOperand(MCOperand::createImm(SysReg.PStateField));
   }
 
   void addSysCROperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     Inst.addOperand(MCOperand::createImm(getSysCR()));
bool AArch64AsmParser::showMatchError(SMLoc Loc, unsigned ErrCode) {
   case Match_InvalidMemoryIndexed2:
     return Error(Loc, "index must be a multiple of 2 in range [0, 8190].");
   case Match_InvalidMemoryIndexed4:
     return Error(Loc, "index must be a multiple of 4 in range [0, 16380].");
   case Match_InvalidMemoryIndexed8:
     return Error(Loc, "index must be a multiple of 8 in range [0, 32760].");
   case Match_InvalidMemoryIndexed16:
     return Error(Loc, "index must be a multiple of 16 in range [0, 65520].");
+  case Match_InvalidImm0_1:
+    return Error(Loc, "immediate must be an integer in range [0, 1].");
   case Match_InvalidImm0_7:
     return Error(Loc, "immediate must be an integer in range [0, 7].");
   case Match_InvalidImm0_15:
     return Error(Loc, "immediate must be an integer in range [0, 15].");
   case Match_InvalidImm0_31:
     return Error(Loc, "immediate must be an integer in range [0, 31].");
   case Match_InvalidImm0_63:
     return Error(Loc, "immediate must be an integer in range [0, 63].");
bool AArch64AsmParser::MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
   case Match_InvalidMemoryXExtend16:
   case Match_InvalidMemoryXExtend32:
   case Match_InvalidMemoryXExtend64:
   case Match_InvalidMemoryXExtend128:
   case Match_InvalidMemoryIndexed4SImm7:
   case Match_InvalidMemoryIndexed8SImm7:
   case Match_InvalidMemoryIndexed16SImm7:
   case Match_InvalidMemoryIndexedSImm9:
+  case Match_InvalidImm0_1:
   case Match_InvalidImm0_7:
   case Match_InvalidImm0_15:
   case Match_InvalidImm0_31:
   case Match_InvalidImm0_63:
   case Match_InvalidImm0_127:
   case Match_InvalidImm0_65535:
   case Match_InvalidImm1_8:
   case Match_InvalidImm1_16:

llvm/trunk/lib/Target/AArch64/Disassembler/AArch64Disassembler.cpp

static DecodeStatus DecodeSystemPStateInstruction(llvm::MCInst &Inst,
                                                   uint32_t insn, uint64_t Addr,
                                                   const void *Decoder) {
   uint64_t op1 = fieldFromInstruction(insn, 16, 3);
   uint64_t op2 = fieldFromInstruction(insn, 5, 3);
   uint64_t crm = fieldFromInstruction(insn, 8, 4);
 
   uint64_t pstate_field = (op1 << 3) | op2;
 
+  if (pstate_field == AArch64PState::PAN && crm > 1)
+    return Fail;
+
   Inst.addOperand(MCOperand::createImm(pstate_field));
   Inst.addOperand(MCOperand::createImm(crm));
 
   bool ValidNamed;
   const AArch64Disassembler *Dis = 
       static_cast<const AArch64Disassembler *>(Decoder);
   (void)AArch64PState::PStateMapper().toString(pstate_field, 
       Dis->getSubtargetInfo().getFeatureBits(), ValidNamed);

llvm/trunk/test/MC/AArch64/armv8.1a-pan.s

 // RUN: not llvm-mc -triple aarch64-none-linux-gnu -mattr=+v8.1a -show-encoding < %s 2> %t | FileCheck %s
 // RUN: FileCheck --check-prefix=CHECK-ERROR %s < %t
 
   .text
 
   msr pan, #0
 // CHECK:  msr PAN, #0           // encoding: [0x9f,0x40,0x00,0xd5]
   msr pan, #1
 // CHECK:  msr PAN, #1           // encoding: [0x9f,0x41,0x00,0xd5]
   msr pan, x5
 // CHECK:  msr PAN, x5           // encoding: [0x65,0x42,0x18,0xd5]
   mrs x13, pan
 // CHECK:  mrs x13, PAN          // encoding: [0x6d,0x42,0x38,0xd5]
 
   msr pan, #-1
-  msr pan, #20
+  msr pan, #2
   msr pan, w0
   mrs w0, pan
-// CHECK-ERROR: error: immediate must be an integer in range [0, 15].
+// CHECK-ERROR: error: immediate must be an integer in range [0, 1].
 // CHECK-ERROR:   msr pan, #-1
 // CHECK-ERROR:            ^
-// CHECK-ERROR: error: immediate must be an integer in range [0, 15].
-// CHECK-ERROR:   msr pan, #20
+// CHECK-ERROR: error: immediate must be an integer in range [0, 1].
+// CHECK-ERROR:   msr pan, #2
 // CHECK-ERROR:            ^
-// CHECK-ERROR: error: immediate must be an integer in range [0, 15].
+// CHECK-ERROR: error: immediate must be an integer in range [0, 1].
 // CHECK-ERROR:   msr pan, w0
 // CHECK-ERROR:            ^
 // CHECK-ERROR: error: invalid operand for instruction
 // CHECK-ERROR:   mrs w0, pan
 // CHECK-ERROR:       ^

llvm/trunk/test/MC/Disassembler/AArch64/armv8.1a-pan.txt

 # RUN: llvm-mc -triple aarch64-none-linux-gnu -mattr=+v8.1a --disassemble < %s | FileCheck %s
 
 0x9f,0x40,0x00,0xd5
 0x9f,0x41,0x00,0xd5
+0x9f,0x42,0x00,0xd5
 0x65,0x42,0x18,0xd5
 0x6d,0x42,0x38,0xd5
 # CHECK:  msr PAN, #0
 # CHECK:  msr PAN, #1
+# CHECK-NOT: msr PAN, #2
 # CHECK:  msr PAN, x5
 # CHECK:  mrs x13, PAN