This is an archive of the discontinued LLVM Phabricator instance.

Use 32/64 SimdDefaultAlign when AVX/AVX512 is enabled, even on i386.
ClosedPublic

Authored by ab on Aug 26 2015, 5:03 PM.

Details

Summary

Per discussion, let's use the features instead of the ABI string.

Original summary:
One problem that came up in D12389 is that i386 doesn't know about the "avx" ABIs. Judging by the commit that originally introduced the X86_64 check and the "avx" ABI (r145652), it was just unnecessary.

Because of that, we can't decide based on the ABI string only for i386.

The only effect this should have is that SimdDefaultAlign would previously always be 128 on i386, no matter the SSE level. We will now use a larger alignment, which seems desirable to me. I added i386 RUN lines to the OpenMP simd test.

I also moved the no-mmx check earlier, as I gather it's necessary for correctness with -mno-mmx (as opposed to the "avx" ABIs which make no difference other than SIMD/vector alignment on i386).

Diff Detail

Event Timeline

ab updated this revision to Diff 33280.Aug 26 2015, 5:03 PM
ab retitled this revision from to Also enable the avx/avx512 ABIs for i386, not just x86_64..
ab updated this object.
ab added a reviewer: rjmccall.
ab added a subscriber: cfe-commits.
rjmccall edited edge metadata.Aug 27 2015, 11:45 AM

This gives no-MMX priority over turning on SSE, which sounds like a major change in behavior to me. There are definitely clients that specifically never want to use MMX but do care deeply about SSE; my understanding is that modern microarchitectures heavily punish casual use of MMX.

ab added a comment.Aug 27 2015, 1:05 PM

Unless I'm misunderstanding, I believe this has much less impact than you're thinking; there are three cases:

  • x86_64: no change (-mno-mmx is guarded by x86)
  • x86, with -mno-mmx: no change (because previously, we'd only set avx/avx512 for x86_64)
  • x86, without -mno-mmx: this will use an avx/avx512 ABI string where we'd have used "", letting us use the better alignment (only for OpenMP and vectors, I think).
In D12390#234458, @ab wrote:

Unless I'm misunderstanding, I believe this has much less impact than you're thinking; there are three cases:

  • x86_64: no change (-mno-mmx is guarded by x86)
  • x86, with -mno-mmx: no change (because previously, we'd only set avx/avx512 for x86_64)
  • x86, without -mno-mmx: this will use an avx/avx512 ABI string where we'd have used "", letting us use the better alignment (only for OpenMP and vectors, I think).

Ok, I see.

The outcome we want is that things like the max vector alignment should be properly honoring whatever target features are actually enabled. Specifically, if AVX is enabled, we should be setting the max vector alignment to the maximum enabled AVX alignment, regardless of whether MMX is enabled.

This ABI string is an artifact of the interface between the AST and IRGen TargetInfos. We should probably just remove it in favor of some kind of openly-extensible query interface so that IRGen can just directly ask things like whether AVX is enabled instead of parsing some random string. But for this patch, let's just leave it as it is, and instead just set the max vector alignment directly from the target features.

This revision was automatically updated to reflect the committed changes.
ab retitled this revision from Also enable the avx/avx512 ABIs for i386, not just x86_64. to Use 32/64 SimdDefaultAlign when AVX/AVX512 is enabled, even on i386..Aug 27 2015, 3:37 PM
ab updated this object.
ab edited edge metadata.

r246228 using the features

Thanks for the reviews!