Does it mean that the OptionalDef operand for Thumb instructions is always at index NumExplicitDefs-1 (rather than at index MCDesc.getNumOperands() - 1)?
Is the OptionalDef already counted as an explicit operand? That's really odd...

This change breaks the assumptions made by the loop at line 348. The assumption is that implicit definitions always follow explicit definitions. By decreasing NumExplicitDefs you are also affecting how that loop iterates over the implicit definitions.

About the code comment: it should mention the issue with the optional def being already counted as an explicit definition.

See my previous comment about the value ot TotalDefs.

Shouldn't we update also TotalDefs? Otherwise we reserve the wrong number of writes at line 312.

Some Thumb instructions (like tMOVSr) have less number of operands in their MCInstrDesc than MCInst.getNumOperands()

Could you elaborate this a bit more? It is a bit concerning that there is a mismatch in the number of operands. What are exactly these extra operands?

I need to understand if the assumptions made by this algorithm are still valid. Specifically, I need to understand if all of the assumptions described in the comment starting at line 256 are still valid. Can we still guarantee that "Uses always start at index #(MCDesc.getNumDefs())"? Is the order of writes still preserved?

After your change, NumExplicitDefs may no longer match the value of MCDesc.getNumDefs().
The value of Index is incorrect computed for Thumb instructions that have an optional def. For those instructions, when CurrentDef is zero, Index references the OptionalDef operand.

Does it mean that the OptionalDef operand for Thumb instructions is always at index NumExplicitDefs-1

Yes, for Thumb1 only. Thumb2 instructions follow ARM rules
Just look at any Thumb1 instruction definition, which modifies CPSR:

def tADC {  // InstructionEncoding
//...
  dag OutOperandList = (outs tGPR:$Rdn, s_cc_out:$s);
//...
}

Unfortunately moving s_cc_out from the end of output operand list to the beginning of input operand list breaks too many things

Is the OptionalDef already counted as an explicit operand? That's really odd.

Yes, because it's in the output operand list

This change breaks the assumptions made by the loop at line 348. The assumption is that implicit definitions always follow explicit definitions.

Thumb1 instructions with optional def do not have implicit operands, however I'll fix this in upcoming update

I think we do a fixup in the end of populateWrites function

ID.Writes.resize(CurrentDef);

Could you elaborate this a bit more? It is a bit concerning that there is a mismatch in the number of operands. What are exactly these extra operands?

If you look at tMOVSr you'll find out that it has 4 operands. For instance movs r2, r3 is assembled to:

<MCInst #4275 tMOVSr <MCOperand Reg:74> <MCOperand Reg:75> <MCOperand Imm:14> <MCOperand Reg:0>>

The last two args are, I beleive, built-in ARMCC::AL predicate

However tMOVSr definition lists only 2 arguments:

def tMOVSr {  // InstructionEncoding
// ...
  dag OutOperandList = (outs tGPR:$Rd);
  dag InOperandList = (ins tGPR:$Rm);
// ...
}

InstrBuilder incorrectly treats tMOVSr as variadic, because 4 != 2

Please don't rely on ThumbFrm, a lot of those flags are legacy and should be removed.

I was hoping that this wouldn't have to say "if (thumb)" anywhere, even if the instructions are a little odd. Can it check the position of the optional def instead? So it detects instructions like this, without having to be target dependent.

Yes. Optional Defs are a little unfortunate, but have been around for a long time and I wouldn't recommend the major changes needed to alter how they work.

This one sounds like a bug to me. Maybe in the AsmParsing? MOVS can't be used in IT blocks, so probably shouldn't use predicated. If you look at PR36658.mir, it looks like $r1 = tMOVSr $r0, implicit-def dead $cpsr

Can it check the position of the optional def instead

Do you have an idea how to do this? One has to distinguish
a) Instruction that writes to single register and optionally modifies CPSR (Thumb1)
b) Instruction that writes to two registers and optionally modifies CPSR (ARM, Thumb2)

This one sounds like a bug to me

May be. I tried to address this in disassembler, but it broke a lot of stuff.

Here is a piece from ARMDisassembler.cpp

// A few instructions actually have predicates encoded in them.  Don't
// try to overwrite it if we're seeing one of those.
switch (MI.getOpcode()) {
  case ARM::tBcc:
  case ARM::t2Bcc:
  case ARM::tCBZ:
  case ARM::tCBNZ:
  case ARM::tCPS:
  case ARM::t2CPS3p:
  case ARM::t2CPS2p:
  case ARM::t2CPS1p:
  case ARM::t2CSEL:
  case ARM::t2CSINC:
  case ARM::t2CSINV:
  case ARM::t2CSNEG:
  case ARM::tMOVSr:
  case ARM::tSETEND:

You are right. That should be enough. Thanks for checking.

Can we just check which operand it is? Use MCDesc.OpInfo[i].isOptionalDef()? Can we just skip it in that case?

It's possibly this code from ARMAsmParser:

TmpInst.setOpcode(Inst.getOperand(4).getReg() ? ARM::tMOVSr : ARM::tMOVr);
TmpInst.addOperand(Inst.getOperand(0));
TmpInst.addOperand(Inst.getOperand(1));
TmpInst.addOperand(Inst.getOperand(2));
TmpInst.addOperand(Inst.getOperand(3));

Do you mind pulling this out into it's own review. I think it's fine, but just in case it probably deserves to be it's own patch.

The test can stay here, so long as there is some test for movs rm, rn.

This comment should be updated now.

By default, the optional register definition is still expected to be the last operand. However, it can also be one of the explicit defs.
The rest of this logic still works under the assumption that instructions can only declare a single optional register definition at most.

Instead of -1, you could just default to MCDesc.getNumOperands() - 1.
That way, the statement at line 381 becomes a simple assignment with no conditional expression.