Loongarch ld instructions have incorrect registers modified

[OBarronCS]

Work environment

Questions	Answers
Capstone module affected	loongarch
Source of Capstone	pip install capstone==6.0.0a4
Version/git commit	6.0.0 alpha 4

Various Loongarch memory load instructions do not indicate the register written in the regs_access() API.

This is the case for

ld.b
ld.h
ld.w
ld.d
ld.bu
ld.hu
ld.wu

cstool -d loongarch64 "0cfe3f2a"
 0  0c fe 3f 2a  ld.bu	$t0, $t4, -1
	ID: 349 (ld_bu)
	op_count: 2
		operands[0].type: REG = t0
		operands[0].access: READ
		operands[1].type: MEM
			operands[1].mem.base: REG = t4
			operands[1].mem.disp: 0xffffffffffffffff
		operands[1].access: READ
	Registers read: t0 t4

Expected behavior

Expected that the first operand is in the Registers modified list. In the case above, this means $t0.

These definitions can be found in LoongArchGenCSMappingInsnOp.inc here: https://github.com/capstone-engine/capstone/blob/fc525c7326a82880e86248d9d1d3a6e9fc6f8750/arch/LoongArch/LoongArchGenCSMappingInsnOp.inc#L3384-L3432
For each of these, the rd (the first) element of the definitions contains CS_AC_READ instead of CS_AC_WRITE

[notxvilka]

cc @jiegec

[jiegec]

Interesting, this code was generated from LLVM auto-sync. Should we edit the LoongArchGenCSMappingInsnOp.inc directly, or we fix the result somewhere else?

[Rot127]

Should we edit the LoongArchGenCSMappingInsnOp.inc directly, or we fix the result somewhere else?

We need to fix it in the LLVM fork. The instruction is defined here.

Weirdly enough the register should be marked as written because it is an out operand of the parent instruction:
https://github.com/capstone-engine/llvm-capstone/blob/095dff8afea49382f935a6f11dd182d313c2a956/llvm/lib/Target/LoongArch/LoongArchInstrInfo.td#L703-L705

[jiegec]

The CS_OP_MEM was generated by the following code in PrinterCapstone.cpp:

  } else if (!InsnPatternMap.empty()) {
    // Pattern field is not set in the CGI.
    // But there might be (multiple) patterns in the record keeper
    // for this CGI
    std::string CGIName = CGI->TheDef->getName().str();
    if (InsnPatternMap.find(CGIName) == InsnPatternMap.end())
      return OperandType;

    bool OpTypeIsPartOfAnyPattern =
        any_of(InsnPatternMap.at(CGIName), [&](Record *PatternDag) {
          return opIsPartOfiPTRPattern(
              OpRec, OpName, PatternDag->getValueAsDag("PatternToMatch"), false,
              true);
        });
    if (OpTypeIsPartOfAnyPattern)
      OperandType += " | CS_OP_MEM"; // this line
    return OperandType;
  }

[jiegec]

@Rot127 Any idea on this? I am unsure how it gets recognized as part of an iPTR pattern.

[Rot127]

@jiegec Sorry for the delay. Just checked and it is indeed the faulty classification of the operand as CS_OP_REG | CS_OP_MEM (instead of just being CS_OP_REG).

Found the issue though. For whatever reason I also classified an operand as memory operand, if it was of the same type as a iPTR operand in the pattern.

In our case this lead to the wrong classification. Because rd (above t0) and tj (above t4) are both of the type GPR.
Removing this wrong assumption generates the correct files.

Here is the diff with the wrong code commented out:

diff --git a/llvm/utils/TableGen/PrinterCapstone.cpp b/llvm/utils/TableGen/PrinterCapstone.cpp
index 2953d4b22a4d..e5df690fc033 100644
--- a/llvm/utils/TableGen/PrinterCapstone.cpp
+++ b/llvm/utils/TableGen/PrinterCapstone.cpp
@@ -3101,11 +3101,11 @@ bool opIsPartOfiPTRPattern(Record const *OpRec, StringRef const &OpName,
     bool Matches;
     StringRef const &PatOpName = PatternDag->getArgNameStr(I);
     Matches = OpName.equals(PatOpName);
-    if (MatchByTypeName) {
-      std::string OpInitType = OpRec->getNameInitAsString();
-      std::string PatOpType = PatternDag->getArg(I)->getAsString();
-      Matches |= OpInitType == PatOpType;
-    }
+    // if (MatchByTypeName) {
+    //   std::string OpInitType = OpRec->getNameInitAsString();
+    //   std::string PatOpType = PatternDag->getArg(I)->getAsString();
+    //   Matches |= OpInitType == PatOpType;
+    // }
     if (MatchByTypeSuperClasses) {
       std::string OpInitType = OpRec->getNameInitAsString();
       std::string PatOpType = PatternDag->getArg(I)->getAsString();

I am pretty busy currently :( Would you mind making a PR to remove these lines? Also removing the unnecessary MatchByTypeName argument?

[jiegec]

I am pretty busy currently :( Would you mind making a PR to remove these lines? Also removing the unnecessary MatchByTypeName argument?

Sure, PR is submitted to capstone-engine/llvm-capstone#79.