[PATCH] D62669: [ARM] Set up infrastructure for MVE vector instructions.

Tue Jun 11 07:59:07 PDT 2019

simon_tatham marked an inline comment as done.
simon_tatham added inline comments.

================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:2183
+      unsigned NextOpIndex = Inst.getNumOperands();
+      const MCInstrDesc &MCID = ARMInsts[Inst.getOpcode()];
+      int TiedOp = MCID.getOperandConstraint(NextOpIndex, MCOI::TIED_TO);
----------------
dmgreen wrote:
> I think this should be using MII.get(Inst.getOpcode()), if it can.
Unfortunately, that's rather fiddly, because the context here is that we're in a method of `ARMOperand`, which doesn't have an `MCInstrInfo` conveniently accessible that I can find – the `ARMAsmParser` as a whole is given one at construction time, but it's not passed through to `ARMOperand`.

I wondered about changing the prototype of this `addVPTPredROperands` method so that it received the `MCInstrDesc` from its caller, and then //that// could look it up in the ambient `MCInstrInfo`. But the caller is the tablegenerated AsmMatcher, and that has hard-coded function call code for all the RenderMethods which expects them to conform to the same API, so that would be a huge piece of plumbing to change.

I think leaving it as it is is safe from a modules perspective. We're referring directly to the `llvm::ARMInsts` array, which is defined in the `LLVMARMDesc` library. But that library also defines the `llvm::ARMMCRegisterClasses` array, which we refer to constantly in this same source file. So anyone linking against this module must surely also have `LLVMARMDesc` available, or else they'd already have had a link error.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62669