[X86] Add a pattern that uses GR16_ABCD rather than GR32_ABCD to avoid falsely marking whole 32 bit register as live.

[kbsmith1]

Summary

This just adds a new pattern for using AH for an 8 bit right shift from a 16 bit value rather than
from a 32 bit value. This prevents use of GR32_ABCD pattern, and a dummy upward conversion.
This makes the register liveness a little more accurate for post register allocation analysis.

[spatel]

[cc'ing Matthias, Mitch, Quentin ; a related test suggestion was made in D20456]

The code change itself looks fine to me, but I wonder if there's a preference for the test case to use "-stop-after" instead (similar to how MIR tests are written):

; RUN: llc -march=x86 -stop-after=expand-isel-pseudos <%s 2>&1 | FileCheck %s

And then either CHECK for the exact expected sequence of machine instructions or CHECK-NOT the instructions that are generated without the patch:

%5 = INSERT_SUBREG %6, %0, 3
%7 = COPY %5

[MatzeB]

In D20649#440928, @spatel wrote:

[cc'ing Matthias, Mitch, Quentin ; a related test suggestion was made in D20456]

The code change itself looks fine to me, but I wonder if there's a preference for the test case to use "-stop-after" instead (similar to how MIR tests are written):

; RUN: llc -march=x86 -stop-after=expand-isel-pseudos <%s 2>&1 | FileCheck %s

And then either CHECK for the exact expected sequence of machine instructions or CHECK-NOT the instructions that are generated without the patch:

%5 = INSERT_SUBREG %6, %0, 3
%7 = COPY %5

In an ideal world the -print-after output and .mir files would look the same. Today I would have a preference to match against the .mir output rather than the -print-machineinstrs output. However in this specific case you run into a shortcoming of .mir: The subregister indexes are printed as numbers rather than their name in the .mir output. So I'd probably keep matching -print-after until this is fixed.

[DavidKreitzer]

The code change looks reasonable to me Kevin.

-Dave

test/CodeGen/X86/i16lshr8pat.ll

8

Really picky, but "16 bit" --> "16-bit" and "32 bit" --> "32-bit"

[spatel]

Given Matthias's feedback on the test, LGTM.

[kbsmith1]

Great, I'll fix the small things Dave pointed out and get this committed.

[qcolombet]

test/CodeGen/X86/i16lshr8pat.ll

2

If you use print-after, I think you need to have an assert build.
Also given what you are checking, wouldn’t the mir test be robust enough?
I mean we do not need to check for the subreg index, do we?

[kbsmith1]

Updated test to use MIR output, and fixed Dave's minor comments on 32-bit and 16-bit.

[kbsmith1]

Addressed comments from Dave & Quentin.

lib/Target/X86/X86InstrCompiler.td

def : Pat<(i8 (trunc GR16:$src)),
           (EXTRACT_SUBREG GR16:$src, sub_8bit)>,
       Requires<[In64BitMode]>;
 
 // h-register tricks
 def : Pat<(i8 (trunc (srl_su GR16:$src, (i8 8)))),
           (EXTRACT_SUBREG (i16 (COPY_TO_REGCLASS GR16:$src, GR16_ABCD)),
                           sub_8bit_hi)>,
       Requires<[Not64BitMode]>;
+def : Pat<(i8 (trunc (srl_su (i32 (anyext GR16:$src)), (i8 8)))),
+          (EXTRACT_SUBREG (i16 (COPY_TO_REGCLASS GR16:$src, GR16_ABCD)),
+                          sub_8bit_hi)>,
+      Requires<[Not64BitMode]>;
 def : Pat<(i8 (trunc (srl_su GR32:$src, (i8 8)))),
           (EXTRACT_SUBREG (i32 (COPY_TO_REGCLASS GR32:$src, GR32_ABCD)),
                           sub_8bit_hi)>,
       Requires<[Not64BitMode]>;
 def : Pat<(srl GR16:$src, (i8 8)),
           (EXTRACT_SUBREG
             (MOVZX32rr8
               (EXTRACT_SUBREG (i16 (COPY_TO_REGCLASS GR16:$src, GR16_ABCD)),

test/CodeGen/X86/i16lshr8pat.ll

+; RUN: llc -march=x86 -stop-after expand-isel-pseudos <%s 2>&1 | FileCheck %s
+
+target datalayout = "e-m:e-p:32:32-f64:32:64-f80:32-n8:16:32-S128"
+target triple = "i386-unknown-linux-gnu"
+
+; This test checks to make sure the lshr in %then1 block gets expanded using
+; GR16_ABCD pattern rather than GR32_ABCD pattern.  By using the 16-bit pattern
+; this doesn't make the register liveness information look like the whole
+; 32-bit register is a live value, and allows generally better live register
+; analysis.
+; CHECK-LABEL: bb.1.then1:
+; CHECK-NOT:   IMPLICIT_DEF
+; CHECK-NOT:   INSERT_SUBREG
+; CHECK:       sub_8bit_hi
+; CHECK-LABEL: bb.2.endif1:
+
+define i16 @foo4(i32 %prec, i8 *%dst, i16 *%src) {
+entry:
+  %cnd = icmp ne i32 %prec, 0
+  %t0 = load i16, i16 *%src, align 2
+  br i1 %cnd, label %then1, label %endif1
+
+then1:
+  %shr = lshr i16 %t0, 8
+  %conv = trunc i16 %shr to i8
+  store i8 %conv, i8 *%dst, align 1
+  br label %endif1
+
+endif1:
+  %t2 = phi i16 [0, %then1], [%t0, %entry]
+  ret i16 %t2
+}