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

[Simon Tatham via Phabricator via llvm-commits]

simon_tatham marked 5 inline comments as done.
simon_tatham added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMInstrVFP.td:2300
   let Inst{3-0}   = 0b0000;
+  let Predicates = [HasVFP2];
 }
----------------
samparker wrote:
> Where has this come from..?
Now you mention it, it looks redundant to me – the base class `VFPI` (via `VFPAI`) sets the same value in any case. Removing it leaves the tablegen diagnostic output unchanged.


================
Comment at: llvm/lib/Target/ARM/ARMRegisterInfo.td:264
+
+def GPRwithZR : RegisterClass<"ARM", [i32], 32, (add (sequence "R%u", 0, 12),
+                                              LR, ZR)> {
----------------
samparker wrote:
> I've just came here from the CSEL instructions, so forgive my limited scope, but why can't this register class access SP?
It's used by the CSEL instructions themselves, and the vector/scalar forms of VCMP and VPT. VCMP can't access sp at all (the ArmARM says "see related encodings"), and all the others are listed as CONSTRAINED UNPREDICTABLE if you try.


================
Comment at: llvm/lib/Target/ARM/ARMRegisterInfo.td:341
+// MVE Condition code register.
+def VCCR : RegisterClass<"ARM", [i32, v16i1, v8i1, v4i1], 32, (add VPR)> {
+//  let CopyCost = -1;  // Don't allow copying of status registers.
----------------
samparker wrote:
> Is the i32 type just to copy us to easily copy the value around?
I didn't write this code personally, but that seems likely to me: it means you can register-allocate an `i32` value into VPR at one point, and into a GPR at another point, and make copy instructions that 'spill' it back and forth, without having to change its type all the time.


================
Comment at: llvm/lib/Target/ARM/ARMScheduleA57.td:98
+
+  let UnsupportedFeatures = [HasV8_1MMainline, HasMVEInt, HasMVEFloat,
+                             HasFPRegsV8_1M];
----------------
dmgreen wrote:
> samparker wrote:
> > Why is this needed? And why just the A57?
> I think this one is because the schedule has CompleteModel = 1 (it may be the only Arm schedule like that). A similar thing was done for SVE on older CPU's in the AArch64 backend not long ago.
Yes – this is the only SchedMachineModel that would otherwise give errors at tablegen time because it doesn't know what all the MVE instructions look like. And I didn't fancy making up a whole set of nonsense about what MVE instructions //do// look like for purposes of scheduling them on a Cortex-A57, for obvious reasons :-)


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62667