This is an archive of the discontinued LLVM Phabricator instance.

[X86] Fix several issues related to X86's psadbw instruction.
ClosedPublic

Authored by congh on Nov 20 2015, 5:20 PM.

Details

Summary

This patch fixes the following issues:

  1. Fix the return type of X86psadbw: it should not be the same type of inputs. For vNi8 inputs the output should be vMi64, where M = N/8.
  2. Fix the return type of int_x86_avx512_psad_bw_512 accordingly.
  3. Fix the definiton of PSADBW, VPSADBW, and VPSADBWY accordingly.
  4. Adjust the return type when building a DAG node of X86ISD::PSADBW type.
  5. Update related tests.

Diff Detail

Repository
rL LLVM

Event Timeline

congh updated this revision to Diff 40851.Nov 20 2015, 5:20 PM
congh retitled this revision from to Fix several issues related to X86's psadbw instruction..
congh updated this object.
congh added reviewers: RKSimon, ashahid, hfinkel, davidxl.
congh added a subscriber: llvm-commits.
congh retitled this revision from Fix several issues related to X86's psadbw instruction. to [X86] Fix several issues related to X86's psadbw instruction..Nov 20 2015, 8:19 PM
RKSimon edited edge metadata.Nov 21 2015, 7:10 AM

Thanks for looking at this! Any idea why there is so much commutation in the tests?

lib/Target/X86/X86InstrSSE.td
4096 ↗(On Diff #40851)

Shouldn't loadv2i64 be memopv2i64?

congh added a comment.Nov 21 2015, 8:19 AM

Thanks for looking at this! Any idea why there is so much commutation in the tests?

I think probably it is because I marked this operation as commutable?

lib/Target/X86/X86InstrSSE.td
4096 ↗(On Diff #40851)

I just copied the following several lines for VPMULUDQ and replaced some strings. I think they are similar, and I am new to this language. I will update it to memopv2i64 if you think this makes more sense.

Thanks for looking at this! Any idea why there is so much commutation in the tests?

I think probably it is because I marked this operation as commutable?

Ah! I see now - only the int_x86_avx2_psad_bw pattern was marked as commutable.

lib/Target/X86/X86InstrSSE.td
4096 ↗(On Diff #40851)

I meant just the SSE implementation (PSADBW) - VPSADBWY can use the loadv*i64 refs - that will match (V)PMULUDQ

congh added inline comments.Nov 22 2015, 12:18 PM
lib/Target/X86/X86InstrSSE.td
4096 ↗(On Diff #40851)

Ah, yes, you are right. Updated. Thanks for pointing it out.

congh updated this revision to Diff 40885.Nov 22 2015, 12:23 PM

Update the patch according to Simon's comment.

AsafBadouh accepted this revision.Nov 24 2015, 1:41 AM
AsafBadouh edited edge metadata.

LGTM

This revision is now accepted and ready to land.Nov 24 2015, 1:41 AM
This revision was automatically updated to reflect the committed changes.