[ARM] processInstruction must return true if it makes a change

[olista01]

Summary

This function must return true if it makes a change, because MatchAndEmitInstruction calls it repeatedly until no further change are made.

No tests because we happen to not hit any of these cases at the moment, but that's just luck.

[rengolin]

We don't hit it by luck is not a good reason. Is it possible to exercise that with a simple MC pattern or not?

Should be simple to add those to existing tests, and it's odd that we have those patterns for years and they have never been tested.

[olista01]

I'm not aware of any test case that can trigger this at the moment, because we get lucky with the ordering of opcodes in the assembly matcher table. However, other patches that I'm working on change that.

[rengolin]

Right, it would be nice to have that as a precursor to the patch set that you're working on, so that you can claim that this is necessary (and show proof with following patches), instead of having it loose like that.

[olista01]

It will probably take me a while to get the patch that uncovers this out, as it's quite complex (adding optional operands in the asm matcher, to remove some hacks like shouldOmitCCOutOperand in the asm parser and get more precise diagnostics), and I'm only working on this as a side project. Since this "bug" isn't observable at the moment I'm fine to leave this in review until the optional operands patch is published, so that it's clear why it is necessary. However, I think it should still be a separate patch, as it's not really related to the optional operands change, and would need to go in before it to avoid test failures.

[rengolin]

It can go in as part of a patch set and it will still avoid the failures.

Sorry, this patch is not obvious enough to just take it at face value.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

if (static_cast<ARMOperand &>(*Operands[0]).getToken() == "push" &&
       TmpInst.setOpcode(ARM::STR_PRE_IMM);
       TmpInst.addOperand(Inst.getOperand(0)); // Rn_wb
       TmpInst.addOperand(Inst.getOperand(4)); // Rt
       TmpInst.addOperand(Inst.getOperand(1)); // addrmode_imm12
       TmpInst.addOperand(MCOperand::createImm(-4));
       TmpInst.addOperand(Inst.getOperand(2)); // CondCode
       TmpInst.addOperand(Inst.getOperand(3));
       Inst = TmpInst;
+      return true;
     }
     break;
   case ARM::t2ADDri12:
     // If the immediate fits for encoding T3 (t2ADDri) and the generic "add"
     // mnemonic was used (not "addw"), encoding T3 is preferred.
     if (static_cast<ARMOperand &>(*Operands[0]).getToken() != "add" ||
         ARM_AM::getT2SOImmVal(Inst.getOperand(2).getImm()) == -1)
       break;
     Inst.setOpcode(ARM::t2ADDri);
     Inst.addOperand(MCOperand::createReg(0)); // cc_out
-    break;
+    return true;
   case ARM::t2SUBri12:
     // If the immediate fits for encoding T3 (t2SUBri) and the generic "sub"
     // mnemonic was used (not "subw"), encoding T3 is preferred.
     if (static_cast<ARMOperand &>(*Operands[0]).getToken() != "sub" ||
         ARM_AM::getT2SOImmVal(Inst.getOperand(2).getImm()) == -1)
       break;
     Inst.setOpcode(ARM::t2SUBri);
     Inst.addOperand(MCOperand::createReg(0)); // cc_out
-    break;
+    return true;
   case ARM::tADDi8:
     // If the immediate is in the range 0-7, we want tADDi3 iff Rd was
     // explicitly specified. From the ARM ARM: "Encoding T1 is preferred
     // to encoding T2 if <Rd> is specified and encoding T2 is preferred
     // to encoding T1 if <Rd> is omitted."
     if ((unsigned)Inst.getOperand(3).getImm() < 8 && Operands.size() == 6) {
       Inst.setOpcode(ARM::tADDi3);
       return true;