This is an archive of the discontinued LLVM Phabricator instance.

[X86][SSE] Improve awareness of (v)cvtpd2ps implicit zeroing of upper 64-bits of xmm result
ClosedPublic

Authored by RKSimon on Aug 23 2016, 5:32 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon updated this revision to Diff 68972.Aug 23 2016, 5:32 AM
RKSimon retitled this revision from to [X86][SSE] Improve awareness of (v)cvtpd2ps implicit zeroing of upper 64-bits of xmm result.
RKSimon updated this object.
RKSimon added reviewers: ab, mkuper, spatel, andreadb.
RKSimon set the repository for this revision to rL LLVM.
RKSimon added a subscriber: llvm-commits.
mkuper edited edge metadata.Aug 29 2016, 1:28 PM

Thanks Simon.

lib/Target/X86/X86InstrSSE.td
2285 ↗(On Diff #68972)

I've only now realized we represent zeroing the two high lanes of a v4f32 with (v4f32 (bitconvert (X86vzmovl (v2f64 (bitconvert (v4f32 ...)))))) :-\
But there's nothing we can really do about this, right?

lib/Target/X86/X86IntrinsicsInfo.h
1887 ↗(On Diff #68972)

This (and the change to the intrinsic test) can be a separate commit, right?
Also, any reason not to add avx_cvt_pd2_ps_256 as well?

RKSimon added inline comments.Aug 29 2016, 2:39 PM
lib/Target/X86/X86InstrSSE.td
2285 ↗(On Diff #68972)

Not much - we use VZEXT_MOVL to zero all but the first vector element.

An alternative would be to have VZEXT32_MOVL and VZEXT64_MOVL (or something similar) - it would affect a lot of existing lowering patterns and I'm it sure its worth it. We have a number of similar bitcasting pattern situation.

lib/Target/X86/X86IntrinsicsInfo.h
1887 ↗(On Diff #68972)

Not reason at all - I'll update it. And yes these could be separate commits.

RKSimon added inline comments.Aug 29 2016, 2:43 PM
lib/Target/X86/X86InstrSSE.td
2285 ↗(On Diff #68972)

An alternative would be to have VZEXT32_MOVL and VZEXT64_MOVL (or something similar) - it would affect a lot of existing lowering patterns and I'm it sure its worth it. We have a number of similar bitcasting pattern situation.

Sorry that should say "and I'm not sure its worth it."

mkuper accepted this revision.Aug 29 2016, 2:47 PM
mkuper edited edge metadata.

LGTM.

lib/Target/X86/X86InstrSSE.td
2285 ↗(On Diff #68972)

Even if we had VZEXT64_MOVL, it wouldn't help (at least, not with what bothers me), we'd still have the ugly casting back and forth.

This revision is now accepted and ready to land.Aug 29 2016, 2:47 PM
This revision was automatically updated to reflect the committed changes.