[PATCH] D60677: [ARM] Rewrite isLegalT2AddressImmediate

[Tim Northover via Phabricator via llvm-commits]

t.p.northover added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:13299
                                       const ARMSubtarget *Subtarget) {
   bool isNeg = false;
   if (V < 0) {
----------------
Variables start with capitals for now.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:13305-13307
+  uint64_t NumBytes = 0;
+  if (VT.isInteger() || VT.isFloatingPoint())
+    NumBytes = VT.getSizeInBits() / 8;
----------------
Early exit would probably be nicer here (if invalid type, return false immediately), and I think it can go at the very top of the function too.

Also, I believe this is incorrect for `i1` (ends up with `NumBytes == 0`).

Finally, this expands the function to cover vector types too. Is that intentional?


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:13314
     V >>= 2;
     return V == (V & ((1LL << 8) - 1));
+  } else if (NumBytes == 1 || NumBytes == 2 || NumBytes == 4) {
----------------
While you're here we should switch this to use `isUInt<8>` from MathExtras.h. Same for the later ones.


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

https://reviews.llvm.org/D60677