[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:251
+bool InstrBuilder::isThumb1Inst(const MCInstrDesc &MCDesc) const {
+  static const unsigned ThumbFrm = 25 << 7; // ARMII::ThumbFrm
+  return STI.getTargetTriple().isThumb() &&
----------------
dmgreen wrote:
> 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.
> 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)


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


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

https://reviews.llvm.org/D91704