[PATCH] D42424: [InstCombine] Allow common type conversions to i8/i16

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:156-157
+  // extra combine opportunities.
+  if (FromLegal && ToWidth < FromWidth && (ToWidth == 8 || ToWidth == 16))
+    return true;
+
----------------
spatel wrote:
> spatel wrote:
> > See Alex Bradbury's comment in the llvm-dev thread about i32 too. Maybe we generalize this for ToLegal as:
> >   bool ToLegal = ToWidth == 1 || isPowerOf2_32(ToWidth) || DL.isLegalInteger(ToWidth);
> pow2 isn't quite right; nobody wants i2 or i4. :)
I took the simple route. Not sure what's really best here. ARM/AArch64 has LDRB/LDRH for loading bytes/halfwords, same for loads, UXTB/UXTH for extends etc. These types feel like they are almost "semi-legal".


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:149
+/// legal to convert to, in order to open up more combining opportunities.
+/// NOTE: this treats i8, i16 and i32 specially, due to them being so common
+/// from frontend languages.
----------------
I almost wrote this as a FIXME:


================
Comment at: test/Transforms/InstCombine/select-bitext.ll:117
+; CHECK-NEXT:    [[SEXT:%.*]] = shl i32 [[TRUNC]], 16
+; CHECK-NEXT:    [[TMP1:%.*]] = ashr exact i32 [[SEXT]], 16
 ; CHECK-NEXT:    [[EXT:%.*]] = select i1 %cmp, i32 [[TMP1]], i32 42
----------------
Although longer, this produced the same final assembly as the old function on any target I tried.


https://reviews.llvm.org/D42424