[PATCH] D62667: [ARM] Add the non-MVE instructions in Arm v8.1-M.

Fri May 31 06:32:53 PDT 2019

samparker added inline comments.

================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp:955
+                   const MCSubtargetInfo &STI) const {
+  // FIXME: The immediate operand should have already been encoded like this
+  // before ever getting here. The encoder method should just need to combine
----------------
simon_tatham wrote:
> samparker wrote:
> > So does this mean we just haven't created, and selected, an operand properly? Why?
> It looks as if that FIXME comment is identical to the one in the previous function, which //wasn't// added by this patch. I guess this function must have been cloned-and-hacked from the previous one with adjustments only to the offset field size.
> 
> As I read the comment, it's saying that it would be nicer to have a different division of work between this level of decoding and whatever created the MC operands that it's starting from. I suppose if anyone ever changes the format of the MC operands as the comment suggests, then all the functions with copies of this FIXME comment will need updating to match.
Ah, the above function wasn't automatically shown in my browser. Then, can we combine these two into a templated function?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62667