This is an archive of the discontinued LLVM Phabricator instance.

Compiler-rt part of D26230: Add (constant) masked load/store support
ClosedPublic

Authored by filcab on Nov 10 2016, 8:05 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

filcab updated this revision to Diff 77488.Nov 10 2016, 8:05 AM
filcab retitled this revision from to Compiler-rt part of D26230: Add (constant) masked load/store support.
filcab updated this object.
filcab added reviewers: kcc, RKSimon, pgousseau.
filcab added a subscriber: llvm-commits.
RKSimon resigned from this revision.Nov 11 2016, 10:56 AM
RKSimon removed a reviewer: RKSimon.
RKSimon added a subscriber: RKSimon.
vitalybuka accepted this revision.Nov 11 2016, 11:01 AM
vitalybuka edited edge metadata.
This revision is now accepted and ready to land.Nov 11 2016, 11:01 AM
This revision was automatically updated to reflect the committed changes.

I've tried pushing this test, but one of the buildbots (at least) doesn't have AVX support. And AVX/AVX2 are the only things on X86 that will generate those masked loads/stores.
I'd go with either:

No testing on compiler-rt (ASan doesn't behave differently in the face of masked ops, so we should be able to go by with only the tests from llvm)
Testing on compiler-rt using IR, *assuming* we can lower calls to masked.loads/stores even on non-AVX CPUs
Testing on compiler-rt using some other arch that's not X86 (I don't really have the hardware to try that out, though)

I've tried pushing this test, but one of the buildbots (at least) doesn't have AVX support. And AVX/AVX2 are the only things on X86 that will generate those masked loads/stores.
I'd go with either:

No testing on compiler-rt (ASan doesn't behave differently in the face of masked ops, so we should be able to go by with only the tests from llvm)
Testing on compiler-rt using IR, *assuming* we can lower calls to masked.loads/stores even on non-AVX CPUs
Testing on compiler-rt using some other arch that's not X86 (I don't really have the hardware to try that out, though)

Can we just add "REQUIRES: avx" to the test and lit?

filcab added a comment.Dec 9 2016, 7:36 AM
In D26506#612095, @kubabrecka wrote:

Can we just add "REQUIRES: avx" to the test and lit?

No. That feature is not detected at all and there's no cmake scaffolding to expose it.
I think this test is too specific, and piggybacking on things that are tested (we're testing the instrumentation itself in llvm-land, callbacks, etc, are the exact same as regular instrumentation).
Since this is a matter of instrument (the exact same way as regular loads/stores) vs. no instrument, I think it's not useful to add all the scaffolding for this test.
If the handling in compiler-rt were in some way different from regular loads/stores, then I think we should add it. But as it is, I'm not sure it's worth the effort.

In D26506#612095, @kubabrecka wrote:

Can we just add "REQUIRES: avx" to the test and lit?

No. That feature is not detected at all and there's no cmake scaffolding to expose it.
I think this test is too specific, and piggybacking on things that are tested (we're testing the instrumentation itself in llvm-land, callbacks, etc, are the exact same as regular instrumentation).
Since this is a matter of instrument (the exact same way as regular loads/stores) vs. no instrument, I think it's not useful to add all the scaffolding for this test.
If the handling in compiler-rt were in some way different from regular loads/stores, then I think we should add it. But as it is, I'm not sure it's worth the effort.

I was thinking about running grep /proc/cpuinfo (and systl hw on Darwin) in lit.cfg to detect AVX. Anyway, I'm fine with dropping this test (and just having the instrumentation test).