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

Fri May 31 08:52:42 PDT 2019

simon_tatham marked 7 inline comments as done.
simon_tatham 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
----------------
samparker wrote:
> 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?
Done, and also, there was another function added later on by D62680 (the load/store instructions) which I've also managed to replace with another instance of this template.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62667