This is an archive of the discontinued LLVM Phabricator instance.

[X86][SSE] Add custom execution domain fixing for BLENDPD/BLENDPS/PBLENDD/PBLENDW (PR34873)
ClosedPublic

Authored by RKSimon on Jan 14 2018, 9:16 AM.

Details

Summary

Add support for custom execution domain fixing and implement support for BLENDPD/BLENDPS/PBLENDD/PBLENDW.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon created this revision.Jan 14 2018, 9:16 AM
craig.topper added inline comments.Jan 14 2018, 11:13 AM
lib/Target/X86/X86InstrInfo.cpp
10083 ↗(On Diff #129784)

Could we just detect ImmWidth == 16 instead of having a Repeat flag?

10085 ↗(On Diff #129784)

Can we mask the immediate to 8-bits on the previous line so we don't need two ands on this line?

10101 ↗(On Diff #129784)

Does this branch only happen when the starting instruction was a BLENDW? And that means that the table is pointing at ReplaceableCustomInstrs not ReplaceableCustomAVX2Instrs?

craig.topper added inline comments.Jan 14 2018, 11:44 AM
lib/Target/X86/X86InstrInfo.cpp
10159 ↗(On Diff #129784)

This line gives a compiler warning due to the assigment not having extra parentheses

craig.topper added inline comments.Jan 14 2018, 12:06 PM
lib/Target/X86/X86InstrInfo.cpp
10098 ↗(On Diff #129784)

This doesn't work if we ever start with a BLENDW that could be represented with a BLENDD right? We'd find the BLENDW in the table search and keep that opcode, but give it a BLENDD immediate?

RKSimon updated this revision to Diff 129788.Jan 14 2018, 12:26 PM

Updated based on Craig's feedback

RKSimon marked 4 inline comments as done.Jan 14 2018, 12:29 PM
RKSimon added inline comments.
lib/Target/X86/X86InstrInfo.cpp
10098 ↗(On Diff #129784)

What should happen is if a PBLENDW mask ends up back here then it can be re-scaled as a PBLENDD, else it'll drop down to PBLENDW again.

craig.topper added inline comments.Jan 14 2018, 12:42 PM
lib/Target/X86/X86InstrInfo.cpp
10098 ↗(On Diff #129784)

But what will change the opcode from PBLENDW to PBLENDD? The table lookup will just hit the original opcode won't it?

RKSimon added inline comments.Jan 14 2018, 1:06 PM
lib/Target/X86/X86InstrInfo.cpp
10098 ↗(On Diff #129784)

No, I cheated and put the VPBLENDWY opcodes under the ReplaceableCustomInstrs not the AVX2 variant, so it'll match in the first lookup (at line 10086) and won't use the second (line 10088). Then if we can widen it for VPBLENDD I lookup again with ReplaceableCustomAVX2Instrs directly

craig.topper added inline comments.Jan 14 2018, 1:14 PM
lib/Target/X86/X86InstrInfo.cpp
10098 ↗(On Diff #129784)

But if the original opcode was VBLENDW/VBLENDWY the lookup into ReplaceableCustomAVX2Instrs at 10098 won't find a match for that opcode and will return null.

RKSimon added inline comments.Jan 14 2018, 1:30 PM
lib/Target/X86/X86InstrInfo.cpp
10098 ↗(On Diff #129784)

Yup you're right - will look into it.

RKSimon updated this revision to Diff 129853.Jan 15 2018, 6:27 AM

Remove rescaling between VPBLENDD and VPBLENDW - it doesn't seem to help as they are both in the integer domain and as @craig.topper noticed was causing issues with the lookup.

This revision is now accepted and ready to land.Jan 15 2018, 11:32 AM
This revision was automatically updated to reflect the committed changes.
RKSimon reopened this revision.Feb 11 2019, 6:43 AM
RKSimon added reviewers: dlj, sammccall.
RKSimon added subscribers: sammccall, dlj.

Reopening as it was reverted at rL353699 due to a rather weird regression.....

@dlj @sammccall Please can you post your repros here?

This revision is now accepted and ready to land.Feb 11 2019, 6:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2019, 6:43 AM
RKSimon closed this revision.Feb 11 2019, 6:44 AM

I hate Mondays - closing this again as its the wrong patch........