This is an archive of the discontinued LLVM Phabricator instance.

[LoongArch] Fix several instruction definition errors in initial patches
ClosedPublic

Authored by SixWeining on Feb 15 2022, 1:21 AM.

Details

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

Diff Detail

Event Timeline

SixWeining created this revision.Feb 15 2022, 1:21 AM
SixWeining requested review of this revision.Feb 15 2022, 1:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2022, 1:21 AM
xen0n added a comment.Feb 15 2022, 1:31 AM

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–216

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 added inline comments.Feb 15 2022, 2:35 AM
llvm/lib/Target/LoongArch/LoongArchInstrFormats.td
263

Thanks. Let's bring it up firstly.

llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
210–216

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 added inline comments.Feb 15 2022, 9:27 AM
llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
210–216

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 added inline comments.Feb 15 2022, 5:04 PM
llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
210–216

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 added inline comments.Feb 15 2022, 7:18 PM
llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
210–216

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 added inline comments.Feb 15 2022, 7:34 PM
llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
210–216

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">;

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

SixWeining retitled this revision from [LoongArch] Fix several instruction definition errors in initial pathes to [LoongArch] Fix several instruction definition errors in initial patches.Feb 15 2022, 7:42 PM
xen0n added inline comments.Feb 15 2022, 7:45 PM
llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
210–216

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 accepted this revision.Feb 15 2022, 7:48 PM

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!

This revision is now accepted and ready to land.Feb 15 2022, 7:48 PM