[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() &&
----------------
evgeny777 wrote:
> 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)
Can we just check which operand it is? Use MCDesc.OpInfo[i].isOptionalDef()? Can we just skip it in that case?


================
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:
> 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:
> ```
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));
```


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

https://reviews.llvm.org/D91704