[LoongArch] Check msb is not less than lsb for the bstr{ins/pick}.{w/d} instructions

[xry111]

It seems the CI failed with some tests TIMEOUT. Maybe we need to adjust the test timeout for the CI.

LoongArch ISA reference manual has not specified the behavior of bstrins/bstrpick with msb < lsb. Should the manual be updated too?

[xry111]

In D124825#3487869, @xry111 wrote:

It seems the CI failed with some tests TIMEOUT. Maybe we need to adjust the test timeout for the CI.

CI passed after I re-ran it.

[SixWeining]

In D124825#3487869, @xry111 wrote:

It seems the CI failed with some tests TIMEOUT. Maybe we need to adjust the test timeout for the CI.

LoongArch ISA reference manual has not specified the behavior of bstrins/bstrpick with msb < lsb. Should the manual be updated too?

Right, the manual doesn’t specify it explicitly. But I think it’s an obvious implication by the instruction explanation. For example:
BSTRINS.W:

bstr32[31:msbw+1] = GR[rd][31: msbw+1]
bstr32[msbw:lsbw] = GR[rj][msbw-lsbw:0]
bstr32[lsbw-1:0] = GR[rd][lsbw-1:0]
GR[rd] = SignExtend(bstr32[31:0], GRLEN)

And this check is also done by GNU as.

Yes, I think it’s better to document it in somewhere.

[SixWeining]

In D124825#3488142, @xry111 wrote:

In D124825#3487869, @xry111 wrote:

It seems the CI failed with some tests TIMEOUT. Maybe we need to adjust the test timeout for the CI.

CI passed after I re-ran it.

Thanks.

[SixWeining]

A polite ping. Thanks.

@xry111 About the ISA document, I will recommend and push the right person to update. But that may not be too fast.

[xry111]

Good to me, but as I've only contributed an one-line change to LLVM let's to wait for someone else :).

[MaskRay]

llvm/test/MC/LoongArch/Basic/Integer/invalid.s

190

Perhaps pick a different set as 1 < 2 has been covered by the previous instruction.

llvm/test/MC/LoongArch/Basic/Integer/invalid64.s

74

ditto

[SixWeining]

Address @MaskRay's comment. Thanks.

[SixWeining]

Thanks, I have updated the patch.

[xen0n]

LGTM, thanks!

llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp

class LoongArchAsmParser : public MCTargetAsmParser {
 
   bool ParseDirective(AsmToken DirectiveID) override { return true; }
 
   bool MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
                                OperandVector &Operands, MCStreamer &Out,
                                uint64_t &ErrorInfo,
                                bool MatchingInlineAsm) override;
 
+  unsigned checkTargetMatchPredicate(MCInst &Inst) override;
+
   unsigned validateTargetOperandClass(MCParsedAsmOperand &Op,
                                       unsigned Kind) override;
 
   bool generateImmOutOfRangeError(OperandVector &Operands, uint64_t ErrorInfo,
                                   int64_t Lower, int64_t Upper, Twine Msg);
 
   /// Helper for processing MC instructions that have been successfully matched
   /// by MatchAndEmitInstruction.
   bool processInstruction(MCInst &Inst, SMLoc IDLoc, OperandVector &Operands,
                           MCStreamer &Out);
 
 // Auto-generated instruction matching functions.
 #define GET_ASSEMBLER_HEADER
 #include "LoongArchGenAsmMatcher.inc"
 
   OperandMatchResultTy parseRegister(OperandVector &Operands);
   OperandMatchResultTy parseImmediate(OperandVector &Operands);
 
   bool parseOperand(OperandVector &Operands, StringRef Mnemonic);
 
 public:
   enum LoongArchMatchResultTy {
     Match_Dummy = FIRST_TARGET_MATCH_RESULT_TY,
+    Match_RequiresMsbNotLessThanLsb,
 #define GET_OPERAND_DIAGNOSTIC_TYPES
 #include "LoongArchGenAsmMatcher.inc"
 #undef GET_OPERAND_DIAGNOSTIC_TYPES
   };
 
   LoongArchAsmParser(const MCSubtargetInfo &STI, MCAsmParser &Parser,
                      const MCInstrInfo &MII, const MCTargetOptions &Options)
       : MCTargetAsmParser(Options, STI, MII) {

 bool LoongArchAsmParser::processInstruction(MCInst &Inst, SMLoc IDLoc,
                                             OperandVector &Operands,
                                             MCStreamer &Out) {
   Inst.setLoc(IDLoc);
   Out.emitInstruction(Inst, getSTI());
   return false;
 }
 
+unsigned LoongArchAsmParser::checkTargetMatchPredicate(MCInst &Inst) {
+  switch (Inst.getOpcode()) {
+  default:
+    break;
+  case LoongArch::BSTRINS_W:
+  case LoongArch::BSTRINS_D:
+  case LoongArch::BSTRPICK_W:
+  case LoongArch::BSTRPICK_D: {
+    unsigned Opc = Inst.getOpcode();
+    const signed Msb =
+        (Opc == LoongArch::BSTRINS_W || Opc == LoongArch::BSTRINS_D)
+            ? Inst.getOperand(3).getImm()
+            : Inst.getOperand(2).getImm();
+    const signed Lsb =
+        (Opc == LoongArch::BSTRINS_W || Opc == LoongArch::BSTRINS_D)
+            ? Inst.getOperand(4).getImm()
+            : Inst.getOperand(3).getImm();
+    if (Msb < Lsb)
+      return Match_RequiresMsbNotLessThanLsb;
+    return Match_Success;
+  }
+  }
+
+  return Match_Success;
+}
+
 unsigned
 LoongArchAsmParser::validateTargetOperandClass(MCParsedAsmOperand &AsmOp,
                                                unsigned Kind) {
   LoongArchOperand &Op = static_cast<LoongArchOperand &>(AsmOp);
   if (!Op.isReg())
     return Match_InvalidOperand;
 
   MCRegister Reg = Op.getReg();
if (Result > FIRST_TARGET_MATCH_RESULT_TY) {
     SMLoc ErrorLoc = IDLoc;
     if (ErrorInfo != ~0ULL && ErrorInfo >= Operands.size())
       return Error(ErrorLoc, "too few operands for instruction");
   }
 
   switch (Result) {
   default:
     break;
+  case Match_RequiresMsbNotLessThanLsb: {
+    SMLoc ErrorStart = Operands[3]->getStartLoc();
+    return Error(ErrorStart, "msb is less than lsb",
+                 SMRange(ErrorStart, Operands[4]->getEndLoc()));
+  }
   case Match_InvalidUImm2:
     return generateImmOutOfRangeError(Operands, ErrorInfo, /*Lower=*/0,
                                       /*Upper=*/(1 << 2) - 1);
   case Match_InvalidUImm2plus1:
     return generateImmOutOfRangeError(Operands, ErrorInfo, /*Lower=*/1,
                                       /*Upper=*/(1 << 2));
   case Match_InvalidUImm3:
     return generateImmOutOfRangeError(Operands, ErrorInfo, /*Lower=*/0,

llvm/test/MC/LoongArch/Basic/Integer/invalid.s

 fadd.s $fa0, $fa0, $fa0
 # CHECK:   :[[#@LINE-1]]:1: error: instruction requires the following: 'F' (Single-Precision Floating-Point)
 fadd.d $fa0, $fa0, $fa0
 # CHECK:   :[[#@LINE-1]]:1: error: instruction requires the following: 'D' (Double-Precision Floating-Point)
 
 ## Using floating point registers when integer registers are expected
 sll.w $a0, $a0, $fa0
 # CHECK: :[[#@LINE-1]]:18: error: invalid operand for instruction
+
+## msbw < lsbw
+# CHECK: :[[#@LINE+1]]:21: error: msb is less than lsb
+bstrins.w $a0, $a0, 1, 2
+# CHECK:            ^~~~
+
+# CHECK: :[[#@LINE+1]]:22: error: msb is less than lsb
+bstrpick.w $a0, $a0, 30, 31
+# CHECK:             ^~~~~~

llvm/test/MC/LoongArch/Basic/Integer/invalid64.s

 addu16i.d $a0, $a0, 32768
 # CHECK: :[[#@LINE-1]]:21: error: immediate must be an integer in the range [-32768, 32767]
 
 ## simm20
 lu32i.d $a0, -0x80001
 # CHECK: :[[#@LINE-1]]:14: error: immediate must be an integer in the range [-524288, 524287]
 pcaddu18i $a0, 0x80000
 # CHECK: :[[#@LINE-1]]:16: error: immediate must be an integer in the range [-524288, 524287]
+
+## msbd < lsbd
+# CHECK: :[[#@LINE+1]]:21: error: msb is less than lsb
+bstrins.d $a0, $a0, 1, 2
+# CHECK:            ^~~~
+
+# CHECK: :[[#@LINE+1]]:22: error: msb is less than lsb
+bstrpick.d $a0, $a0, 32, 63
+# CHECK:             ^~~~~~