This is an archive of the discontinued LLVM Phabricator instance.

[x86] Fix lowering to illegal type in LowerINSERT_VECTOR_ELT
ClosedPublic

Authored by xiangzhangllvm on Jul 25 2021, 8:52 PM.

Details

Summary

A bugfix for X86TargetLowering:
For INSERT_VECTOR_ELT, X86TargetLowering may try to generate illegal type, for example v2i64 in special 32-bits target.
Because this Lowering optimization is done after Type Legalization, so we can no longer convert illegal type to legal again.

The small reproduce case "vector_double2_insert.ll" will crash in X86TargetLowering::LowerINSERT_VECTOR_ELT

Diff Detail

Event Timeline

xiangzhangllvm created this revision.Jul 25 2021, 8:52 PM
xiangzhangllvm requested review of this revision.Jul 25 2021, 8:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2021, 8:52 PM
xiangzhangllvm edited the summary of this revision. (Show Details)Jul 26 2021, 1:41 AM
xiangzhangllvm edited the summary of this revision. (Show Details)

Nice catch!

llvm/test/CodeGen/X86/vector_double2_insert.ll
2 ↗(On Diff #361573)

We'd probably be better off adding i686 triple coverage to llvm/test/CodeGen/X86/avx-insertelt.ll to get much better coverage:

; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+avx  | FileCheck %s --check-prefixes=ALL,AVX
; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+avx2 | FileCheck %s --check-prefixes=ALL,AVX2,X64-AVX2
; RUN: llc < %s -mtriple=i686-unknown-unknown -mattr=+avx2 | FileCheck %s --check-prefixes=ALL,AVX2,X86-AVX2
RKSimon added inline comments.Jul 26 2021, 2:38 AM
llvm/test/CodeGen/X86/vector_double2_insert.ll
2 ↗(On Diff #361573)

Sorry that should be llvm/test/CodeGen/X86/insertelement-var-index.ll - add at least one AVX2 or later RUN with i686 triple:

; RUN: llc < %s -mtriple=x86_64-- -mattr=+avx2   | FileCheck %s --check-prefixes=ALL,AVX,AVX1OR2,AVX2,X64-AVX2
; RUN: llc < %s -mtriple=i686--   -mattr=+avx2   | FileCheck %s --check-prefixes=ALL,AVX,AVX1OR2,AVX2,X86-AVX2

Thanks @RKSimon 's review, I remove my old test, add check x86avx2 in insertelement-var-index.ll (It also can reproduce the bug)

Because the 32bit output has a lot of difference with the 64, I independently use "check x86avx2" for it.

xiangzhangllvm marked 2 inline comments as done.Jul 26 2021, 6:39 PM
xiangzhangllvm added inline comments.
llvm/test/CodeGen/X86/vector_double2_insert.ll
2 ↗(On Diff #361573)

There already "-mtriple=x86_64-- -mattr=+avx2 " here, so add "-mtriple=i686-- -mattr=+avx2" thanks!

RKSimon accepted this revision.Jul 27 2021, 1:29 AM

LGTM with one minor change

llvm/test/CodeGen/X86/insertelement-var-index.ll
8

Can you make it --check-prefixes=ALL,X86AVX2 please?

This revision is now accepted and ready to land.Jul 27 2021, 1:29 AM
xiangzhangllvm marked an inline comment as done.Jul 27 2021, 1:43 AM
xiangzhangllvm added inline comments.
llvm/test/CodeGen/X86/insertelement-var-index.ll
8

Of course, just need change "retq" to "ret", thanks a lot!

Refine test checks in insertelement-var-index.ll (other changes was auto modified by utils/update_llc_test_checks.py)

xiangzhangllvm marked an inline comment as done.Jul 27 2021, 1:55 AM

Refine test checks in insertelement-var-index.ll (other changes was auto modified by utils/update_llc_test_checks.py)

I think this can be updated by the script.

RKSimon added inline comments.Jul 27 2021, 2:20 AM
llvm/test/CodeGen/X86/insertelement-var-index.ll
8

yup - the script should do that for you

Yes, It did it, let me commit it after the latest check-all testing, thanks!

This revision was landed with ongoing or failed builds.Jul 27 2021, 5:17 PM
This revision was automatically updated to reflect the committed changes.