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

Fri Mar 22 12:03:46 PDT 2019

efriedma added inline comments.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2129
+  case AArch64::LDRSHWui:
+  case AArch64::LDRSHXui:
   case AArch64::STRHui:
----------------
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.

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

https://reviews.llvm.org/D59635