[PATCH] D54803: [x86] promote all multiply i8 by constant to i32

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked an inline comment as done.
spatel added inline comments.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:41066-41077
+  // i16 is legal, but undesirable since i16 instruction encodings are longer
+  // and some i16 instructions are slow.
+  if (VT != MVT::i16) {
+    // 8-bit multiply is probably not much cheaper than 32-bit multiply, and
+    // we have specializations to turn 32-bit multiply into LEA or other ops.
+    if (VT != MVT::i8 || Opc != ISD::MUL)
+      return true;
----------------
lebedev.ri wrote:
> I'm trying to parse this and i'm failing.
> 
> The old code was:
> * If the type is not `i16`, then it is desirable. (so i8/i32/i64)
> * Else, see `switch`.
> 
> New code says:
> * If the type is not `i16` (so i8/i32/i64)
>   and either
>   * the type is not `i8` (so i32/i64)
>   or
>   * [the type is `i8` and] the opcode is not `ISD::MUL`
>   then it is desirable
> * Else, see `switch`.
> 
> Doesn't this not only mark i8 MUL's as undesirable, but also mark all other i8 as desirable?
> 
> Can this code please at least be uncondenced?
Your description sounds right - all i8 ops besides mul are still desirable - and that's the intended logic. I think it's just a DeMorgan illusion that makes it confusing (and I probably got it wrong in my initial draft locally!). I'll rearrange it to try to avoid that problem.


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

https://reviews.llvm.org/D54803