[llvm] r227983 - Fix program crashes due to alignment exceptions generated for SSE memop instructions (PR22371).

Demikhovsky, Elena elena.demikhovsky at intel.com
Thu Feb 5 13:12:38 PST 2015


It broke folding unaligned loads in instructions. 

-  Elena


-----Original Message-----
From: hwennborg at google.com [mailto:hwennborg at google.com] On Behalf Of Hans Wennborg
Sent: Thursday, February 05, 2015 22:52
To: Demikhovsky, Elena
Cc: Sanjay Patel; Craig Topper; llvm-commits; Adam Nemet (anemet at apple.com); Kuperstein, Michael M; Nadav Rotem
Subject: Re: [llvm] r227983 - Fix program crashes due to alignment exceptions generated for SSE memop instructions (PR22371).

I don't have enough expertise in this area of the code to quite understand what's broken. As far as I understand, Sanjay's patch fixed most of PR22374. What aspect of AVX-512 did it break?

On Thu, Feb 5, 2015 at 12:03 PM, Demikhovsky, Elena <elena.demikhovsky at intel.com> wrote:
> I know that the AVX-512 will be incorrect with this patch. I can add a 
> test that will fail, but I can’t rewrite in one day all AVX-512 instructions.
>
>
>
> -           Elena
>
>
>
> From: Sanjay Patel [mailto:spatel at rotateright.com]
> Sent: Thursday, February 05, 2015 18:03
> To: Demikhovsky, Elena
> Cc: Craig Topper; llvm-commits; Adam Nemet (anemet at apple.com); 
> Kuperstein, Michael M; Nadav Rotem; Hans W
>
>
> Subject: Re: [llvm] r227983 - Fix program crashes due to alignment 
> exceptions generated for SSE memop instructions (PR22371).
>
>
>
> The intent of r227983 is simply to revert the wrong behavior 
> introduced by
> r224330 for *SSE* codegen.
> If r224330 was supposed to change behavior of AVX512, it should have 
> included test cases to validate that, right? These should be separate 
> patches.
>
>> You removed FeatureVectorUAMem form HSW, but you did not add 
>> FeatureSSEUnalignedMem
>
> Correct - AFAIK, Haswell doesn't have this feature. Perhaps we need to 
> change 'memop' to 'sse_memop' to make it clearer that memops are only 
> for use with SSE...or just modify the 'load' defs to include this 
> predicate
> somehow:
>
> // Like 'load', but uses special alignment checks suitable for use in 
> // memory operands in most SSE instructions, which are required to // 
> be naturally aligned on some targets but not on others.  If the 
> subtarget // allows unaligned accesses, match any load, though this 
> may require // setting a feature bit in the processor (on startup, for example).
> // Opteron 10h and later implement such a feature.
> def memop : PatFrag<(ops node:$ptr), (load node:$ptr), [{
>   return    Subtarget->hasSSEUnalignedMem()
>          || cast<LoadSDNode>(N)->getAlignment() >= 16; }]>;
>
>
>
>
>
> On Thu, Feb 5, 2015 at 1:11 AM, Demikhovsky, Elena 
> <elena.demikhovsky at intel.com> wrote:
>
> It can use load, I agree.
>
> But AVX512 code should be changed before committing the patch.
>
>
>
> -           Elena
>
>
>
> From: Craig Topper [mailto:craig.topper at gmail.com]
> Sent: Thursday, February 05, 2015 10:04
>
>
> To: Demikhovsky, Elena
> Cc: Sanjay Patel; llvm-commits
> Subject: Re: [llvm] r227983 - Fix program crashes due to alignment 
> exceptions generated for SSE memop instructions (PR22371).
>
>
>
> Why is AVX512 not using load?
>
>
>
> On Wed, Feb 4, 2015 at 11:59 PM, Demikhovsky, Elena 
> <elena.demikhovsky at intel.com> wrote:
>
> We use it in AVX512.
>
>
>
> -           Elena
>
>
>
> From: Craig Topper [mailto:craig.topper at gmail.com]
> Sent: Thursday, February 05, 2015 09:52
> To: Demikhovsky, Elena
> Cc: Sanjay Patel; llvm-commits
>
>
> Subject: Re: [llvm] r227983 - Fix program crashes due to alignment 
> exceptions generated for SSE memop instructions (PR22371).
>
>
>
> I thought we had converted all of AVX/AVX2 to use 'loadXXX' instead of 
> 'memopXXX'?
>
>
>
> On Wed, Feb 4, 2015 at 11:44 PM, Demikhovsky, Elena 
> <elena.demikhovsky at intel.com> wrote:
>
> Hi,
> You removed FeatureVectorUAMem form HSW, but you did not add 
> FeatureSSEUnalignedMem
>                                       FeatureAVX2,
>                                       FeatureCMPXCHG16B,
>                                       FeatureFastUAMem,
> -                                     FeatureVectorUAMem,
>                                       FeaturePOPCNT,
>                                       FeatureAES,
>                                       FeaturePCLMUL,
>
> How memop should work now on AVX/AVX2? Will it require alignment now?
>
> def memop : PatFrag<(ops node:$ptr), (load node:$ptr), [{
> -  return    Subtarget->hasVectorUAMem()
> +  return    Subtarget->hasSSEUnalignedMem()
>           || cast<LoadSDNode>(N)->getAlignment() >= 16;  }]>;
>
> -  Elena
>
>
> -----Original Message-----
> From: Kuperstein, Michael M
> Sent: Wednesday, February 04, 2015 21:41
> To: Hans Wennborg; Nadav Rotem; Demikhovsky, Elena
> Cc: Sanjay Patel; llvm-commits
> Subject: RE: [llvm] r227983 - Fix program crashes due to alignment 
> exceptions generated for SSE memop instructions (PR22371).
>
> I understand this on roughly the same level as Nadav does (looks 
> correct, and should be safer than the current situation), but Elena 
> should be able to give an authoritative answer.
> Elena, could you comment?
>
> Michael
>
> -----Original Message-----
> From: hwennborg at google.com [mailto:hwennborg at google.com] On Behalf Of 
> Hans Wennborg
> Sent: Wednesday, February 04, 2015 20:53
> To: Nadav Rotem
> Cc: Sanjay Patel; llvm-commits; Demikhovsky, Elena; Kuperstein, 
> Michael M
> Subject: Re: [llvm] r227983 - Fix program crashes due to alignment 
> exceptions generated for SSE memop instructions (PR22371).
>
> Elena, Michael: is this safe for merging to 3.6?
>
> As far as I understand it's basically a revert or r224330.
>
> On Tue, Feb 3, 2015 at 3:44 PM, Nadav Rotem <nrotem at apple.com> wrote:
>> The change looks good to me, but I would wait a day and let Elena or 
>> Michael review it.
>>
>> On Feb 3, 2015, at 3:19 PM, Hans Wennborg <hans at chromium.org> wrote:
>>
>> On Tue, Feb 3, 2015 at 9:13 AM, Sanjay Patel <spatel at rotateright.com>
>> wrote:
>>
>> Author: spatel
>> Date: Tue Feb  3 11:13:04 2015
>> New Revision: 227983
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=227983&view=rev
>> Log:
>> Fix program crashes due to alignment exceptions generated for SSE 
>> memop instructions (PR22371).
>>
>> r224330 introduced a bug by misinterpreting the "FeatureVectorUAMem" bit.
>> The commit log says that change did not affect anything, but that's 
>> not correct.
>> That change allowed SSE instructions to have unaligned mem operands 
>> folded into math ops, and that's not allowed in the default 
>> specification for any SSE variant.
>>
>> The bug is exposed when compiling for an AVX-capable CPU that had 
>> this feature flag but without enabling AVX codegen. Another mistake 
>> in
>> r224330 was not adding the feature flag to all AVX CPUs; the AMD 
>> chips were excluded.
>>
>> This is part of the fix for PR22371 (
>> http://llvm.org/bugs/show_bug.cgi?id=22371 ).
>>
>> This feature bit is SSE-specific, so I've renamed it to 
>> "FeatureSSEUnalignedMem".
>> Changed the existing test case for the feature bit to reflect the new 
>> name and renamed the test file itself to better reflect the feature.
>> Added runs to fold-vex.ll to check for the failing codegen.
>>
>> Note that the feature bit is not set by default on any CPU because it 
>> may require a configuration register setting to enable the enhanced 
>> unaligned behavior.
>>
>>
>> Sanjay requested in [1] that we should merge this to the 3.6 branch.
>> I'm not qualified to determine if this is safe or not. Is there 
>> somone else with X86 expertise that would like to chime in? Nadav, 
>> you're listed as the code owner here.
>>
>> Thanks,
>> Hans
>>
>> 1. http://llvm.org/bugs/show_bug.cgi?id=22374#c3
>>
>>
> ---------------------------------------------------------------------
> Intel Israel (74) Limited
>
> This e-mail and any attachments may contain confidential material for 
> the sole use of the intended recipient(s). Any review or distribution 
> by others is strictly prohibited. If you are not the intended 
> recipient, please contact the sender and delete all copies.
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
>
>
> --
>
> ~Craig
>
> ---------------------------------------------------------------------
> Intel Israel (74) Limited
>
> This e-mail and any attachments may contain confidential material for 
> the sole use of the intended recipient(s). Any review or distribution 
> by others is strictly prohibited. If you are not the intended 
> recipient, please contact the sender and delete all copies.
>
>
>
>
>
> --
>
> ~Craig
>
> ---------------------------------------------------------------------
> Intel Israel (74) Limited
>
> This e-mail and any attachments may contain confidential material for 
> the sole use of the intended recipient(s). Any review or distribution 
> by others is strictly prohibited. If you are not the intended 
> recipient, please contact the sender and delete all copies.
>
>
>
> ---------------------------------------------------------------------
> Intel Israel (74) Limited
>
> This e-mail and any attachments may contain confidential material for 
> the sole use of the intended recipient(s). Any review or distribution 
> by others is strictly prohibited. If you are not the intended 
> recipient, please contact the sender and delete all copies.
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.




More information about the llvm-commits mailing list