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

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/MCA/InstrBuilder.cpp:251
+bool InstrBuilder::isThumb1Inst(const MCInstrDesc &MCDesc) const {
+  static const unsigned ThumbFrm = 25 << 7; // ARMII::ThumbFrm
+  return STI.getTargetTriple().isThumb() &&
----------------
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.


================
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())
----------------
evgeny777 wrote:
> 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
> 
> 
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.


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


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

https://reviews.llvm.org/D91704