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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:1719
+  switch (Opc) {
+  default: return 0;
+  case AArch64::PRFMui: return AArch64::PRFUMi;
----------------
paquette wrote:
> 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.
I've used Optional<unsigned> to make it more clear to the caller that the value can be invalid. Do you think the name-change is still needed?


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2129
+  case AArch64::LDRSHWui:
+  case AArch64::LDRSHXui:
   case AArch64::STRHui:
----------------
efriedma wrote:
> sdesmalen wrote:
> > 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?
> It tends to be easier for everyone when non-functional changes are split from functional changes, in cases where it isn't too difficult.
> 
> I'm fine committing an "obvious" change like this without a test, though; writing a separate test for each possible opcode just to reproduce the information here isn't really productive.
Right, I have now split the 'functional change' (the added case statements) into a separate patch that I plan to commit to trunk directly.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:3278
+
+  Offset += Remainder;
   if (OutUseUnscaledOp)
----------------
sdesmalen wrote:
> 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?
I have refactored/simplified the math a bit more, let me know if you think this clarified things.


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

https://reviews.llvm.org/D59635