[PATCH] D120476: [LoongArch] Add basic support to AsmParser

Fri Feb 25 01:28:16 PST 2022

SixWeining added a comment.

I have splited the `EncoderMethod` changes to D120545 <https://reviews.llvm.org/D120545>. Thanks!

================
Comment at: llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCCodeEmitter.cpp:81
+unsigned
+LoongArchMCCodeEmitter::getImmOpValueSub1(const MCInst &MI, unsigned OpNo,
+                                          SmallVectorImpl<MCFixup> &Fixups,
----------------
myhsu wrote:
> SixWeining wrote:
> > myhsu wrote:
> > > Why did you include code emitter changes here? can you split these (and the `EncoderMethod` changes in the tablegen files) into another patch?
> > These 2 methods are used by assembler to encode instruction's `unusual` immediate operand.  The word `unusual` means the encoding of an immediate in instruction is not the same as its binary representation. For example,  the `bl` instruction requires a signed 28-bits integer as its operand and the low 2-bits must be 0. So assembler only need to encode the uppper 26-bits into instruction's encoding.
> > 
> > So I think these changes could not be moved otherwise the MC test will fail.
> but without these two instructions to encode `unusual`, why didn't the previous version of tests (.mir files) also fail?
Good question! I haven't realized this problem.
The previous .mir test uses incorrect MIR, for example:

```
llvm/test/CodeGen/LoongArch/misc.mir

# -------------------------------------------------------------------------------------------------
#                                           Encoding format: I26
# -------------------------------------------------------------------------------------------------
# ------------------+-----------------------------------------------+------------------------------
#  31 30 29 28 27 26 25 24 23 22 21 20 19 18 17 16 15 14 13 12 11 10 09 08 07 06 05 04 03 02 01 00
# ------------------+-----------------------------------------------+------------------------------
#     opcode        |                   imm26{15-0}                 |       imm26{25-16}
# ------------------+-----------------------------------------------+------------------------------

---
# CHECK-LABEL: test_BL:
# CHECK-ENC: 0 1 0 1 0 1 0 0 0 0 0 0 0 0 0 0 1 0 0 0 1 0 0 0 0 0 0 0 0 0 0 0
# CHECK-ASM: bl 34
name: test_BL
body: |
  bb.0:
    BL 34
...
```

Here, `34`  is an illegal immediate operand for the `BL` instruction. The reason why it doesn't fails is `getImmOpValueAsr2` is not added at that time.

So, I think the right way is just as you said "split these (and the EncoderMethod changes in the tablegen files) into another patch" and fix the .mir test.

Thanks again!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120476