This is an archive of the discontinued LLVM Phabricator instance.

[AVX512][FastISel] Do not use K registers in TEST instructions
ClosedPublic

Authored by guyblank on Aug 4 2016, 6:57 AM.

Details

Summary

since i1 is legal in AVX512, i1 values weren't promoted to i8, instead K registers were allocated.
this causes an issue when they are passed to a TEST instruction, which is illegal.

Solves Bug 28661

while this patch solves this specific issue, what I would really want to do is make the isel choose GPRs for these i1 values.

Diff Detail

Event Timeline

guyblank updated this revision to Diff 66799.Aug 4 2016, 6:57 AM
guyblank retitled this revision from to [AVX512][FastISel] Do not use K registers in TEST instructions.
guyblank updated this object.
guyblank added reviewers: delena, igorb.
guyblank added a subscriber: llvm-commits.
delena edited edge metadata.Aug 4 2016, 12:13 PM

Any test?

delena added inline comments.Aug 4 2016, 12:37 PM
lib/Target/X86/X86FastISel.cpp
1694

It may give incorrect result. If the OpReg is k-reg you can use k-shitft-left 15 with ktestw.

guyblank updated this revision to Diff 68013.Aug 15 2016, 4:12 AM
guyblank edited edge metadata.

as Elena suggested, changed the sequence to kshift left & ktest.
added a test.

igorb added inline comments.Aug 16 2016, 3:30 AM
lib/Target/X86/X86FastISel.cpp
1705

KTEST instruction require AVX512DQ, it will fail to run (illegal instruction) on knl target.
We assume that VK1 is always zero extended, you don't need shift.
I suggest to use KORTESTW instruction ( it require only AVX512F , available if VK1 legal ), in this case it have the same semantic.

test/CodeGen/X86/fast-isel-select-cmov.ll
1 ↗(On Diff #68013)

could you please change it to
; RUN: llc < %s -fast-isel -fast-isel-abort=1 -mtriple=x86_64-apple-darwin10 | FileCheck %s --check-prefix=CHECK --check-prefix=NOAVX512
; RUN: llc < %s -fast-isel -fast-isel-abort=1 -mtriple=x86_64-apple-darwin10 -mattr=+avx512f | FileCheck %s --check-prefix=CHECK --check-prefix=AVX512

and regenerate the checks using update_llc_test_checks utils.

guyblank updated this revision to Diff 68496.Aug 18 2016, 1:42 AM

changes according to igor's comments

guyblank marked 2 inline comments as done.Aug 18 2016, 1:43 AM
igorb added inline comments.Aug 18 2016, 4:31 AM
lib/Target/X86/X86FastISel.cpp
2052

could you please check if this one should be addReg(CondReg, getKillRegState(CondIsKill)) ?

2236

could you please check if this one should be addReg(CondReg, getKillRegState(CondIsKill))?

guyblank updated this revision to Diff 68537.Aug 18 2016, 7:51 AM

add kill state

guyblank marked 2 inline comments as done.Aug 18 2016, 7:52 AM
igorb accepted this revision.Aug 18 2016, 9:06 AM
igorb edited edge metadata.

LGTM

This revision is now accepted and ready to land.Aug 18 2016, 9:06 AM
This revision was automatically updated to reflect the committed changes.