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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen marked 3 inline comments as done.
sdesmalen added inline comments.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2129
+  case AArch64::LDRSHWui:
+  case AArch64::LDRSHXui:
   case AArch64::STRHui:
----------------
efriedma wrote:
> It seems unlikely to me that this actually has no effect on the generated code; getMemOpInfo has other callers.
Good spot! Having a look how this function is used (mostly in `getOutliningCandidateInfo` and also `getMemOperandWithOffset`, which is then used in e.g. `MachineSink`), it will probably be a bit awkward to make tests for it.  So just to double check, are you suggesting to pull out these changes to a separate patch that adds tests for different users/uses of this function, or are you suggesting to drop the 'NFC' from the description?


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:3278
+
+  Offset += Remainder;
   if (OutUseUnscaledOp)
----------------
efriedma wrote:
> The math here is weird... could you check whether the offset is valid in some more obvious way?
Do you mean any lines in specific? Or the entire way we calculate (Emittable)Offset and Remainder here?


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.h:91
+  /// Returns the index for the immediate for a given instruction.
+  static unsigned getImmIdx(unsigned Opc);
+
----------------
efriedma wrote:
> This could use a better name, to reflect how you expect it to be used, e.g. getLoadStoreImmIdx?  (It probably isn't right for all instructions...)
You're right, this is a bit of misnomer. getLoadStoreImmIdx sounds more appropriate! I'll change that in the next revision.


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

https://reviews.llvm.org/D59635