This is an archive of the discontinued LLVM Phabricator instance.

[X86][AVX512] Make i1 illegal in the CodeGen
ClosedPublic

Authored by guyblank on Apr 20 2017, 12:28 AM.

Details

Summary

Following this RFC.
This patch defines the i1 type as illegal in the X86 backend for AVX512.
for DAG operations on <N x i1> types (build vector, extract vector element, ...) i8 is used, and should be truncated/extended.
A v1i1 MVT is added to support scalar instruction that use masks, like floating point compare.

Diff Detail

Event Timeline

guyblank created this revision.Apr 20 2017, 12:28 AM
guyblank retitled this revision from [X86][AVX512] to [X86][AVX512] Make i1 illegal in the CodeGen.
craig.topper edited edge metadata.Apr 20 2017, 4:18 PM

I still need to look at this some more, but here a few comments.

include/llvm/CodeGen/MachineValueType.h
58 ↗(On Diff #95897)

This needs to be rebased on top of a commit that went in today that changed this for ScalableVector MVTs.

448 ↗(On Diff #95897)

Can you put the scalar type before the vector type? That would be more consistent with the other code.

lib/Target/X86/X86InstrInfo.cpp
6318 ↗(On Diff #95897)

Why is this code coming back? We should use the right SUBREG operations in the output patterns so that we only do GR32/GR64 copies.

guyblank updated this revision to Diff 96536.Apr 25 2017, 5:47 AM

updates after comments by @craig.topper

Should the v1i1 MVT be added in a separate patch?

Should the v1i1 MVT be added in a separate patch?

sure, i'll separate it.

Should the v1i1 MVT be added in a separate patch?

sure, i'll separate it.

D32540 for the MVT change.

craig.topper added inline comments.Apr 28 2017, 12:19 PM
lib/Target/X86/X86CallingConv.td
75–76

Update comment

76

Merge with the line below and have 4 types?

148–149

Update comment

149

Shouldn't this be merge with the line below to list 3 types?

489–490

Update comment.

490–491

Isn't the original line redundant?

588–589

Update comment

589–590

Isn't the old line here redundant?

800–801

Fix the comment

801–802

Isn't the second line here redundant?

821

Merge with the line below and fix comment

833

Merge with the line below and fix comment

863

Merge with the line below and fix comment

870

Merge with the line below and fix comment.

890

Merge with the line below and fix comment

lib/Target/X86/X86ISelLowering.cpp
6905

Why did this if statement need to change?

13951

What's the different between this and the original line? Doesn't Op's VT have to match the elements of the vector?

19034

This seems like an unrelated cleanup. Commit separately?

Why are curly braces being added?

29529

Can we reuse CondVT here instead of calling Cond.getValueType() again?

Why cant' we use DAG.getAllOnesConstant anymore? Does the constant have different with than CondVT's element type now? What does that do?

lib/Target/X86/X86InstrAVX512.td
2303

Align v16i1 to same column as v8i1 above.

2316

The identation on the VK16 seems off. Same with VK8 on the pattern below.

lib/Target/X86/X86InstrInfo.cpp
6333 ↗(On Diff #96536)

Not sure why there are two blanks lines in the current code, but don't delete the extra one in this patch.

guyblank updated this revision to Diff 98098.May 7 2017, 4:06 AM
guyblank marked 21 inline comments as done.

address craig's comments

guyblank updated this revision to Diff 98266.May 9 2017, 4:05 AM
guyblank added inline comments.
lib/Target/X86/X86ISelLowering.cpp
6905

just leftovers from other changes i had here which weren't needed.

13951

since i1 is illegal, Op VT would be i8 when extracting from i1 vectors.

it is ok for the return type of extract vector element to be wider than the vector elements.

29529

getAllOnesConstant probably just got lost in a merge. there is no issue with it

I think this looks ok to me. @RKSimon what do you think?

RKSimon added inline comments.May 13 2017, 5:54 AM
test/CodeGen/X86/avx512-intrinsics.ll
125

Any ideas what is going on here?

guyblank added inline comments.May 17 2017, 6:20 AM
test/CodeGen/X86/avx512-intrinsics.ll
125

SelectionDAG.cpp::FoldConstantArithmetic should eliminate this.
but there is a comment there...
Avoid BUILD_VECTOR nodes that perform implicit truncation.
FIXME: This is valid and could be handled by truncation.

without this patch the build vector was v16i1 = build vector i1, i1, ...
and with the patch it is v16i1 = build vector i8, i8, ...
so now it is unable to eliminate the xnor.

i've tried implementing proper handling for implicit truncation but i'm getting some failure in mips that i still need to investigate.
is it ok if I fix it in a separate commit?

RKSimon accepted this revision.May 17 2017, 11:29 AM

LGTM - I'm happy for the missed constant fold to be handled in a followup, add a TODO if you can.

test/CodeGen/X86/avx512-intrinsics.ll
125

SelectionDAG::FoldConstantVectorArithmetic does the explicit truncation/extension - not sure if you can use that.

This revision is now accepted and ready to land.May 17 2017, 11:29 AM
guyblank added inline comments.May 18 2017, 4:33 AM
test/CodeGen/X86/avx512-intrinsics.ll
125

Thanks!

This revision was automatically updated to reflect the committed changes.
test/CodeGen/X86/avx512-load-store.ll