[LoongArch] Fix several instruction definition errors in initial patches

[SixWeining]

Summary

This patch corrects some instruction definitions that I incorrectly wrote
in initial patches including bstr{ins/pick}.{w/d}, ll.{w/d} and sc.{w/d}.

Depends on D119813

[xen0n]

Side comment: now you see why I insisted on auto-generating instruction definitions, so this kind of typo wouldn't happen in the first place ;-)

llvm/lib/Target/LoongArch/LoongArchInstrFormats.td

263

Okay so this is being changed to exactly match manual syntax. While I believe the semantics is not affected at all, it's okay nevertheless, because at this point everything else matches manual description. (However ugly/inconsistent that is; but again, changing these is outside the scope of the initial bring-up.)

llvm/lib/Target/LoongArch/LoongArchInstrInfo.td

146

Fair enough, the original description is clearly wrong.

210–218

So every other line in this section are simple references to already-nicely-wrapped templates, except these two? I think keeping the previous style of writing would be nicer.

Also, is the constraint meant to cover the first def only? I guess you may want to cover both, so this may need revision too.

[SixWeining]

llvm/lib/Target/LoongArch/LoongArchInstrFormats.td

263

Thanks. Let's bring it up firstly.

llvm/lib/Target/LoongArch/LoongArchInstrInfo.td

210–218

No, the "let Constraints" is meant to only cover the first def. This is because as the ISA mannul describes "rd" is not only used as output but also input for bstrins.* instructions while "rd" is only used as output for bstrpick.* instructions. So the "$rd = $dst" constraint should only cover bstrins.*.

About the "nice format", I also tried to keep the privious style, however, with above reason, I have to define 2 different instruction formats for bstrins and bstrpick. Like this:

let Constraints = "$rd = $dst" in
class ALU_BSTRINSW<bits<12> op, string opstr, Operand ImmOpnd>
    : FmtBSTR_W<op, (outs GPR:$dst),
                (ins GPR:$rd, GPR:$rj, ImmOpnd:$msbw, ImmOpnd:$lsbw),
                !strconcat(opstr, "\t$rd, $rj, $msbw, $lsbw")>;
class ALU_BSTRPICKW<bits<12> op, string opstr, Operand ImmOpnd>
    : FmtBSTR_W<op, (outs GPR:$rd), (ins GPR:$rj, ImmOpnd:$msbw, ImmOpnd:$lsbw),
                !strconcat(opstr, "\t$rd, $rj, $msbw, $lsbw")>;

Then define bstrins.w and bstrpick.w like this:

def BSTRINS_W  : ALU_BSTRINSW<0b000000000110, "bstrins.w", uimm5>;
def BSTRPICK_W : ALU_BSTRPICKW<0b000000000111, "bstrpick.w", uimm5>;

Due to below reasons I give up this approach:

1. These 2 classes (ALU_BSTRINSW and ALU_BSTRPICKW) are based on the FmtBSTR_W class and only used once seperately. So tt's not necessary to wrap the FmtBSTR_W again.
2. Maybe it's hard to keep same style. For example, the BL already uses the FmtI26 format but not a re-wrapped format.

def BL : FmtI26<0b010101, (outs), (ins simm26_lsl2:$imm26), "bl\t$imm26">;

[myhsu]

llvm/lib/Target/LoongArch/LoongArchInstrInfo.td

210–218

I think the author not only want to add a new constraint but also changing the input / output operands. Otherwise we can simply write this

let Constraints = "$rd = $dst" in
def BSTRINS_W  : ALU_BSTRW<0b000000000110, "bstrins.w", uimm5>;
def BSTRPICK_W : ALU_BSTRW<0b000000000111, "bstrpick.w", uimm5>;

[SixWeining]

llvm/lib/Target/LoongArch/LoongArchInstrInfo.td

210–218

Yes, bstrins.w and bstrpick.w use different input / output operands. This is the reason why they cannot use a shared class with same outs and ins.

[xen0n]

llvm/lib/Target/LoongArch/LoongArchInstrInfo.td

210–218

Okay I get the reasoning and they are acceptable to me. However I think you could use an extra newline to clear any confusion; otherwise even developers relatively experienced in LoongArch (such as me) could misunderstand. Because you have every "group" of instructions start with a comment line already (e.g. the // Bit-manipulation Instructions line above), additional newlines wouldn't hurt.

[SixWeining]

llvm/lib/Target/LoongArch/LoongArchInstrInfo.td

210–218

That sounds good. Seems there is another approach:

let Constraints = "$rd = $dst" in {
def BSTRINS_D  : FmtBSTR_D<0b0000000010, (outs GPR:$dst),
                           (ins GPR:$rd, GPR:$rj, uimm6:$msbd, uimm6:$lsbd),
                           "bstrins.d\t$rd, $rj, $msbd, $lsbd">;
}
def BSTRPICK_D : FmtBSTR_D<0b0000000011, (outs GPR:$rd),
                           (ins GPR:$rj, uimm6:$msbd, uimm6:$lsbd),
                           "bstrpick.d\t$rd, $rj, $msbd, $lsbd">;

[SixWeining]

Add extra '{' and '}' to avoid confusions.

[xen0n]

llvm/lib/Target/LoongArch/LoongArchInstrInfo.td

210–218

Yeah just pick whatever you find aesthetically pleasing. I grepped the sources for similar usages (let XXX = XXX in (a single def)), and it seems people prefer omitting the braces. But braces are explicit and IMO better than newlines if bracing single def is considered okay.

[xen0n]

With the braces added to clearly convey intention, I think this is good to go, but I'd of course leave it to other (more experienced) maintainers to decide.

Thanks for the fix!

llvm/lib/Target/LoongArch/LoongArchInstrFormats.td

class FmtI26<bits<6> op, dag outs, dag ins, string asmstr,
   bits<26> imm26;
 
   let Inst{31-26} = op;
   let Inst{25-10} = imm26{15-0};
   let Inst{9-0} = imm26{25-16};
 }
 
 // FmtBSTR_W
-// <opcode[11:1] | msb | opcode[0] | lsb | rj | rd>
+// <opcode[11:1] | msbw | opcode[0] | lsbw | rj | rd>
 class FmtBSTR_W<bits<12> op, dag outs, dag ins, string asmstr,
                 list<dag> pattern = []> : LAInst<outs, ins, asmstr, pattern> {
-  bits<5> msb;
-  bits<5> lsb;
+  bits<5> msbw;
+  bits<5> lsbw;
   bits<5> rj;
   bits<5> rd;
 
   let Inst{31-21} = op{11-1};
-  let Inst{20-16} = msb;
+  let Inst{20-16} = msbw;
   let Inst{15} = op{0};
-  let Inst{14-10} = lsb;
+  let Inst{14-10} = lsbw;
   let Inst{9-5} = rj;
   let Inst{4-0} = rd;
 }
 
 // FmtBSTR_D
-// <opcode | msb | lsb | rj | rd>
+// <opcode | msbd | lsbd | rj | rd>
 class FmtBSTR_D<bits<10> op, dag outs, dag ins, string asmstr,
                 list<dag> pattern = []> : LAInst<outs, ins, asmstr, pattern> {
-  bits<6> msb;
-  bits<6> lsb;
+  bits<6> msbd;
+  bits<6> lsbd;
   bits<5> rj;
   bits<5> rd;
 
   let Inst{31-22} = op;
-  let Inst{21-16} = msb;
-  let Inst{15-10} = lsb;
+  let Inst{21-16} = msbd;
+  let Inst{15-10} = lsbd;
   let Inst{9-5} = rj;
   let Inst{4-0} = rd;
 }
 
 // FmtASRT
 // <opcode | rk | rj | 0x0>
 class FmtASRT<bits<17> op, dag outs, dag ins, string asmstr,
               list<dag> pattern = []> : LAInst<outs, ins, asmstr, pattern> {

llvm/lib/Target/LoongArch/LoongArchInstrInfo.td

: Fmt2RI12<op, (outs GPR:$rd), (ins GPR:$rj, ImmOpnd:$imm12),
                !strconcat(opstr, "\t$rd, $rj, $imm12")>;
 class ALU_2RI16<bits<6> op, string opstr, Operand ImmOpnd>
     : Fmt2RI16<op, (outs GPR:$rd), (ins GPR:$rj, ImmOpnd:$imm16),
                !strconcat(opstr, "\t$rd, $rj, $imm16")>;
 class ALU_1RI20<bits<7> op, string opstr, Operand ImmOpnd>
     : Fmt1RI20<op, (outs GPR:$rd), (ins ImmOpnd:$imm20),
                !strconcat(opstr, "\t$rd, $imm20")>;
 
-class ALU_BSTRW<bits<12> op, string opstr, Operand ImmOpnd>
-    : FmtBSTR_W<op, (outs GPR:$rd), (ins GPR:$rj, ImmOpnd:$msb, ImmOpnd:$lsb),
-                !strconcat(opstr, "\t$rd, $rj, $msb, $lsb")>;
-class ALU_BSTRD<bits<10> op, string opstr, Operand ImmOpnd>
-    : FmtBSTR_D<op, (outs GPR:$rd), (ins GPR:$rj, ImmOpnd:$msb, ImmOpnd:$lsb),
-                !strconcat(opstr, "\t$rd, $rj, $msb, $lsb")>;
-
 class MISC_I15<bits<17> op, string opstr>
     : FmtI15<op, (outs), (ins uimm15:$imm15), !strconcat(opstr, "\t$imm15")>;
 
 class RDTIME_2R<bits<22> op, string opstr>
     : Fmt2R<op, (outs GPR:$rd, GPR:$rj), (ins),
                  !strconcat(opstr, "\t$rd, $rj")>;
 
 class BrCC_2RI16<bits<6> op, string opstr>

 
 let mayLoad = 1, mayStore = 1 in
 class AM_3R<bits<17> op, string opstr>
     : Fmt3R<op, (outs GPR:$rd), (ins GPR:$rj, GPR:$rk),
             !strconcat(opstr, "\t$rd, $rj, $rk")>;
 
 let mayLoad = 1 in
 class LLBase<bits<8> op, string opstr>
-    : Fmt2RI14<op, (outs GPR:$rd), (ins GPR:$rj, simm14:$imm14),
+    : Fmt2RI14<op, (outs GPR:$rd), (ins GPR:$rj, simm14_lsl2:$imm14),
                !strconcat(opstr, "\t$rd, $rj, $imm14")>;
 
 let mayStore = 1, Constraints = "$rd = $dst" in
 class SCBase<bits<8> op, string opstr>
-    : Fmt2RI14<op, (outs GPR:$dst), (ins GPR:$rd, GPR:$rj, simm14:$imm14),
+    : Fmt2RI14<op, (outs GPR:$dst), (ins GPR:$rd, GPR:$rj, simm14_lsl2:$imm14),
                !strconcat(opstr, "\t$rd, $rj, $imm14")>;
 
 //===----------------------------------------------------------------------===//
 // Instructions
 //===----------------------------------------------------------------------===//
 
 // Arithmetic Operation Instructions
 def ADD_W : ALU_3R<0b00000000000100000, "add.w">;

 def CLO_W   : ALU_2R<0b0000000000000000000100, "clo.w">;
 def CLZ_W   : ALU_2R<0b0000000000000000000101, "clz.w">;
 def CTO_W   : ALU_2R<0b0000000000000000000110, "cto.w">;
 def CTZ_W   : ALU_2R<0b0000000000000000000111, "ctz.w">;
 def BYTEPICK_W : ALU_3RI2<0b000000000000100, "bytepick.w", uimm2>;
 def REVB_2H   : ALU_2R<0b0000000000000000001100, "revb.2h">;
 def BITREV_4B : ALU_2R<0b0000000000000000010010, "bitrev.4b">;
 def BITREV_W  : ALU_2R<0b0000000000000000010100, "bitrev.w">;
-def BSTRINS_W  : ALU_BSTRW<0b000000000110, "bstrins.w", uimm5>;
-def BSTRPICK_W : ALU_BSTRW<0b000000000111, "bstrpick.w", uimm5>;
+let Constraints = "$rd = $dst" in {
+def BSTRINS_W  : FmtBSTR_W<0b000000000110, (outs GPR:$dst),
+                           (ins GPR:$rd, GPR:$rj, uimm5:$msbw, uimm5:$lsbw),
+                           "bstrins.w\t$rd, $rj, $msbw, $lsbw">;
+}
+def BSTRPICK_W : FmtBSTR_W<0b000000000111, (outs GPR:$rd),
+                           (ins GPR:$rj, uimm5:$msbw, uimm5:$lsbw),
+                           "bstrpick.w\t$rd, $rj, $msbw, $lsbw">;
 def MASKEQZ : ALU_3R<0b00000000000100110, "maskeqz">;
 def MASKNEZ : ALU_3R<0b00000000000100111, "masknez">;
 
 // Branch Instructions
 def BEQ  : BrCC_2RI16<0b010110, "beq">;
 def BNE  : BrCC_2RI16<0b010111, "bne">;
 def BLT  : BrCC_2RI16<0b011000, "blt">;
 def BGE  : BrCC_2RI16<0b011001, "bge">;
 def BLTU : BrCC_2RI16<0b011010, "bltu">;

 def BYTEPICK_D : ALU_3RI3<0b00000000000011, "bytepick.d", uimm3>;
 def REVB_4H   : ALU_2R<0b0000000000000000001101, "revb.4h">;
 def REVB_2W   : ALU_2R<0b0000000000000000001110, "revb.2w">;
 def REVB_D    : ALU_2R<0b0000000000000000001111, "revb.d">;
 def REVH_2W   : ALU_2R<0b0000000000000000010000, "revh.2w">;
 def REVH_D    : ALU_2R<0b0000000000000000010001, "revh.d">;
 def BITREV_8B : ALU_2R<0b0000000000000000010011, "bitrev.8b">;
 def BITREV_D  : ALU_2R<0b0000000000000000010101, "bitrev.d">;
-def BSTRINS_D  : ALU_BSTRD<0b0000000010, "bstrins.d", uimm6>;
-def BSTRPICK_D : ALU_BSTRD<0b0000000011, "bstrpick.d", uimm6>;
+let Constraints = "$rd = $dst" in {
+def BSTRINS_D  : FmtBSTR_D<0b0000000010, (outs GPR:$dst),
+                           (ins GPR:$rd, GPR:$rj, uimm6:$msbd, uimm6:$lsbd),
+                           "bstrins.d\t$rd, $rj, $msbd, $lsbd">;
+}
+def BSTRPICK_D : FmtBSTR_D<0b0000000011, (outs GPR:$rd),
+                           (ins GPR:$rj, uimm6:$msbd, uimm6:$lsbd),
+                           "bstrpick.d\t$rd, $rj, $msbd, $lsbd">;
 
 // Common Memory Access Instructions for 64-bits
 def LD_WU : LOAD_2RI12<0b0010101010, "ld.wu">;
 def ST_D : STORE_2RI12<0b0010100111, "st.d">;
 def LDX_B  : LOAD_3R<0b00111000000000000, "ldx.b">;
 def LDX_H  : LOAD_3R<0b00111000000001000, "ldx.h">;
 def LDX_W  : LOAD_3R<0b00111000000010000, "ldx.w">;
 def LDX_D  : LOAD_3R<0b00111000000011000, "ldx.d">;

llvm/test/CodeGen/LoongArch/misc.mir

 
 ---
 # CHECK-LABEL: test_BSTRINS_W:
 # CHECK-ENC: 0 0 0 0 0 0 0 0 0 1 1 0 0 1 1 1 0 0 0 0 1 0 0 0 1 0 1 0 0 1 0 0
 # CHECK-ASM: bstrins.w  $a0, $a1, 7, 2
 name: test_BSTRINS_W
 body: |
   bb.0:
-    $r4 = BSTRINS_W $r5, 7, 2
+    $r4 = BSTRINS_W $r4, $r5, 7, 2
 ...
 ---
 # CHECK-LABEL: test_BSTRPICK_W:
 # CHECK-ENC: 0 0 0 0 0 0 0 0 0 1 1 0 1 0 1 0 1 0 0 1 0 0 0 0 1 0 1 0 0 1 0 0
 # CHECK-ASM: bstrpick.w $a0, $a1, 10, 4
 name: test_BSTRPICK_W
 body: |
   bb.0:

 
 ---
 # CHECK-LABEL: test_BSTRINS_D:
 # CHECK-ENC: 0 0 0 0 0 0 0 0 1 0 0 0 0 1 1 1 0 0 0 0 1 0 0 0 1 0 1 0 0 1 0 0
 # CHECK-ASM: bstrins.d  $a0, $a1, 7, 2
 name: test_BSTRINS_D
 body: |
   bb.0:
-    $r4 = BSTRINS_D $r5, 7, 2
+    $r4 = BSTRINS_D $r4, $r5, 7, 2
 ...
 ---
 # CHECK-LABEL: test_BSTRPICK_D:
 # CHECK-ENC: 0 0 0 0 0 0 0 0 1 1 1 0 0 1 1 1 0 1 0 1 1 0 0 0 1 0 1 0 0 1 0 0
 # CHECK-ASM: bstrpick.d $a0, $a1, 39, 22
 name: test_BSTRPICK_D
 body: |
   bb.0: