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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb 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())
----------------
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.


================
Comment at: llvm/lib/MCA/InstrBuilder.cpp:308
+  if (IsThumb && MCDesc.hasOptionalDef())
+    NumExplicitDefs--;
+
----------------
Shouldn't we update also `TotalDefs`? Otherwise we reserve the wrong number of writes at line 312.



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


================
Comment at: llvm/lib/MCA/InstrBuilder.cpp:312
+      MCDesc.isVariadic() ? MCI.getNumOperands() - MCDesc.getNumOperands() : 0;
   ID.Writes.resize(TotalDefs + NumVariadicOps);
   // Iterate over the operands list, and skip non-register operands.
----------------
See my previous comment about the value ot `TotalDefs`.


================
Comment at: llvm/lib/MCA/InstrBuilder.cpp:349
   for (CurrentDef = 0; CurrentDef < NumImplicitDefs; ++CurrentDef) {
     unsigned Index = NumExplicitDefs + CurrentDef;
     WriteDescriptor &Write = ID.Writes[Index];
----------------
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.



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

https://reviews.llvm.org/D91704