[PATCH] D119817: [LoongArch] Fix several instruction definition errors in initial pathes

[Lu Weining via Phabricator via llvm-commits]

SixWeining added inline comments.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchInstrFormats.td:263
+  bits<5> msbw;
+  bits<5> lsbw;
   bits<5> rj;
----------------
xen0n wrote:
> 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.)
Thanks. Let's bring it up firstly.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchInstrInfo.td:210-216
+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">;
----------------
xen0n wrote:
> 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.
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">;
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119817/new/

https://reviews.llvm.org/D119817