[PATCH] D91704: [llvm-mca] Fix processing thumb instruction set

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 added inline comments.


================
Comment at: llvm/lib/MCA/InstrBuilder.cpp:305-306
 
-  unsigned NumVariadicOps = MCI.getNumOperands() - MCDesc.getNumOperands();
+  bool IsThumb = STI.getTargetTriple().isThumb();
+  // Thumb target specifies CPSR (s_cc_out) as an output operand
+  if (IsThumb && MCDesc.hasOptionalDef())
----------------
andreadb wrote:
> 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.
> 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




================
Comment at: llvm/lib/MCA/InstrBuilder.cpp:308
+  if (IsThumb && MCDesc.hasOptionalDef())
+    NumExplicitDefs--;
+
----------------
andreadb wrote:
> Shouldn't we update also `TotalDefs`? Otherwise we reserve the wrong number of writes at line 312.
> 
I think we do a fixup in the end of populateWrites function
```
ID.Writes.resize(CurrentDef);
```


================
Comment at: llvm/lib/MCA/InstrBuilder.cpp:310-311
+
+  unsigned NumVariadicOps =
+      MCDesc.isVariadic() ? MCI.getNumOperands() - MCDesc.getNumOperands() : 0;
   ID.Writes.resize(TotalDefs + NumVariadicOps);
----------------
andreadb wrote:
> > 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?
> 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



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

https://reviews.llvm.org/D91704