This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by kbsmith1 on May 25 2016, 4:04 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

kbsmith1 updated this revision to Diff 58530.May 25 2016, 4:04 PM
kbsmith1 retitled this revision from to [X86] Add a pattern that uses GR16_ABCD rather than GR32_ABCD to avoid falsely marking whole 32 bit register as live..
kbsmith1 updated this object.
kbsmith1 added reviewers: DavidKreitzer, spatel.
kbsmith1 added a subscriber: llvm-commits.

[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 edited edge metadata.May 26 2016, 11:30 AM

[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.

The code change looks reasonable to me Kevin.

-Dave

test/CodeGen/X86/i16lshr8pat.ll
7 ↗(On Diff #58530)

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

spatel accepted this revision.May 26 2016, 1:43 PM
spatel edited edge metadata.

Given Matthias's feedback on the test, LGTM.

This revision is now accepted and ready to land.May 26 2016, 1:43 PM

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

qcolombet added inline comments.May 26 2016, 4:22 PM
test/CodeGen/X86/i16lshr8pat.ll
1 ↗(On Diff #58530)

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 this revision to Diff 58848.May 27 2016, 3:24 PM
kbsmith1 edited edge metadata.

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

kbsmith1 marked 2 inline comments as done.May 27 2016, 3:25 PM

Addressed comments from Dave & Quentin.

This revision was automatically updated to reflect the committed changes.