This is an archive of the discontinued LLVM Phabricator instance.

[X86][MMX] Added custom lowering action for MMX SELECT (PR30418)
ClosedPublic

Authored by kbelochapka on Jun 26 2017, 6:05 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

kbelochapka created this revision.Jun 26 2017, 6:05 PM
RKSimon edited edge metadata.Jun 27 2017, 4:03 AM

Thanks for looking at this!

Very minor, but can you make sure that the patch title is something like "[X86][MMX] Added custom lowering action for MMX SELECT (PR30418)" and then make your current title the summary - it makes finding reviews/commits easier.

test/CodeGen/X86/pr30418.ll
5 ↗(On Diff #104054)

If possible, we need better test coverage here. Please can you rename the file select-mmx.ll, rename this test pr30418 and add filecheck and update_llc_test_checks.py codegen generation.

You also need to test on a i686 triple as well since you're creating i64 values.

A few extra tests wouldn't go amiss - select with non-constant mmx values for instance.

kbelochapka retitled this revision from Fix for pr30418 - error in backend: Cannot select: t17: x86mmx = select_cc t2, Constant:i64<0>, t7, t8, seteq:ch to [X86][MMX] Added custom lowering action for MMX SELECT (PR30418).Jul 12 2017, 6:28 PM
kbelochapka edited the summary of this revision. (Show Details)

fix now works for 32bit mode, the test updated from crash checking to positive

This diff looks like it has gone bad being rebased?

test/CodeGen/X86/select-mmx.ll
2 ↗(On Diff #106368)

Use triples + attributes instead of arch/cpu (avoids test failure on different machines):

; RUN: llc -mtriple=x86_64-unknown-unknown -mattr=+mmx < %s | FileCheck %s --check-prefix=X64
; RUN: llc -mtriple=i686-unknown-unknown -mattr=+mmx < %s | FileCheck %s --check-prefix=I32

replaced llc option -mcpu for -mtriple, the diff file reformatted

RKSimon added inline comments.Jul 14 2017, 1:49 AM
lib/Target/X86/X86ISelLowering.cpp
427 ↗(On Diff #106504)

This diff is still broken - I'm assuming it should be before this for loop and after the loop with the other SELECT/SETCC declarations?

30573 ↗(On Diff #106504)

Again, the diff is broken - its embedded in the middle of combineCMov's constant folding code.....

updated poorly formated diff file

Code now looks fine, just some further tidying up of the test cases

test/CodeGen/X86/select-mmx.ll
2 ↗(On Diff #106699)

You should regenerate the codegen with utils/update_llc_test_checks.py

12 ↗(On Diff #106699)

You can drop 'local_unnamed_addr #1' and probably add 'nonuwind' instead

49 ↗(On Diff #106699)

Remove the #3

62 ↗(On Diff #106699)

You can drop 'local_unnamed_addr #1' and probably add 'nonuwind' instead

97 ↗(On Diff #106699)

Remove the #3

102 ↗(On Diff #106699)

Remove the #2

test regenerated with update_llc_test_checks.py, unnecessary artifacts removed.

RKSimon accepted this revision.Jul 19 2017, 2:26 AM

LGTM - thanks

This revision is now accepted and ready to land.Jul 19 2017, 2:26 AM
This revision was automatically updated to reflect the committed changes.