[PATCH] D59635: [AArch64] NFC: Cleanup isAArch64FrameOffsetLegal

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:1719
+  switch (Opc) {
+  default: return 0;
+  case AArch64::PRFMui: return AArch64::PRFUMi;
----------------
Maybe this could use a better name? It's unintuitive to return 0 when the name seems to imply that you expect a scaled load/store opcode. I would expect this to be unreachable with the current name.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:3244
 
-  case AArch64::LDURXi:
-  case AArch64::LDURWi:
-  case AArch64::LDURBi:
-  case AArch64::LDURHi:
-  case AArch64::LDURSi:
-  case AArch64::LDURDi:
-  case AArch64::LDURQi:
-  case AArch64::LDURHHi:
-  case AArch64::LDURBBi:
-  case AArch64::LDURSBXi:
-  case AArch64::LDURSBWi:
-  case AArch64::LDURSHXi:
-  case AArch64::LDURSHWi:
-  case AArch64::LDURSWi:
-  case AArch64::STURXi:
-  case AArch64::STURWi:
-  case AArch64::STURBi:
-  case AArch64::STURHi:
-  case AArch64::STURSi:
-  case AArch64::STURDi:
-  case AArch64::STURQi:
-  case AArch64::STURBBi:
-  case AArch64::STURHHi:
-    Scale = 1;
-    break;
-  }
+  unsigned UnscaledOp = AArch64InstrInfo::getUnscaledLdSt(MI.getOpcode());
+  unsigned ImmIdx = AArch64InstrInfo::getImmIdx(MI.getOpcode());
----------------
Probably worth a comment saying that this can be 0 here.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.h:86
 
+  /// Returns the unscaled load/store for the scaled load/store opcode.
+  static unsigned getUnscaledLdSt(unsigned Opc);
----------------
Probably good to note that this returns 0 if the opcode doesn't correspond to a scaled load or store.


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

https://reviews.llvm.org/D59635