[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/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:
> SixWeining wrote:
> > myhsu wrote:
> > > SixWeining wrote:
> > > > 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">;
> > > > ```
> > > 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>;
> > > ```
> > 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`.
> 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.
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">;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119817