This is an archive of the discontinued LLVM Phabricator instance.

[x86/SLH] Teach speculative load hardening to correctly harden the indices used by AVX2 and AVX-512 gather instructions.
ClosedPublic

Authored by chandlerc on Jul 13 2018, 9:35 PM.

Details

Summary

The index vector is hardened by broadcasting the predicate state
into a vector register and then or-ing. We don't even have to worry
about EFLAGS here.

I've added a test for all of the gather intrinsics to make sure that we
don't miss one. A particularly interesting creation is the gather
prefetch, which needs to be marked as potentially "loading" to get the
correct behavior. It's a memory access in many ways, and is actually
relevant for SLH. However, if marking as mayLoad seems to
heavy-handed, I can change how I find loads to specifically look for
prefetches.

Diff Detail

Repository
rL LLVM

Event Timeline

chandlerc created this revision.Jul 13 2018, 9:35 PM
chandlerc updated this revision to Diff 155557.Jul 14 2018, 3:20 AM
chandlerc edited the summary of this revision. (Show Details)

Update to use a much better testing strategy, and include tests of all the
AVX512 gather instructions in addition to the AVX2 ones.

This also enhances the rewrite rules to use AVX512VL instructions for rewriting
128-bit and 256-bit gathers when those instructions are available. This should
allow them to have access to the extra registers, etc. It also allows them to
work correctly when the gather itself already is using the AVX512VL register
classes.

chandlerc updated this revision to Diff 155602.Jul 15 2018, 4:46 PM

Rebased and tests updated.

craig.topper added inline comments.Jul 15 2018, 5:15 PM
llvm/lib/Target/X86/X86InstrAVX512.td
9591 ↗(On Diff #155602)

I wonder if this should be mayLoad=1, mayStore =1, hasSideEffects = 0. That's what we use for other prefetch instructions.

chandlerc added inline comments.Jul 15 2018, 5:33 PM
llvm/lib/Target/X86/X86InstrAVX512.td
9591 ↗(On Diff #155602)

You tell me. I'm happy with whatever you think makes sense here really.

craig.topper added inline comments.Jul 15 2018, 5:56 PM
llvm/lib/Target/X86/X86InstrAVX512.td
9591 ↗(On Diff #155602)

Yeah lets go with mayLoad=1 mayStore=1. I think that's what we would have inferred based on how the intrinsic is defined.

chandlerc updated this revision to Diff 155604.Jul 15 2018, 6:05 PM

Fix suggested in review.

chandlerc marked 3 inline comments as done.Jul 15 2018, 6:06 PM
chandlerc added inline comments.
llvm/lib/Target/X86/X86InstrAVX512.td
9591 ↗(On Diff #155602)

Done.

This revision is now accepted and ready to land.Jul 15 2018, 8:52 PM
This revision was automatically updated to reflect the committed changes.
chandlerc marked an inline comment as done.