[llvm] r227983 - Fix program crashes due to alignment exceptions generated for SSE memop instructions (PR22371).
Craig Topper
craig.topper at gmail.com
Thu Feb 5 15:44:32 PST 2015
Will the folding tables in X86InstrInfo somewhat mitigate this? I'm sure it
won't be perfect, but maybe offers some protection.
On Thu, Feb 5, 2015 at 1:29 PM, Hans Wennborg <hans at chromium.org> wrote:
> Is it a miscompile (i.e. it's now folding to something illegal) or a
> performance (failing to fold to something that would be better) problem?
>
> - Hans
>
> On Thu, Feb 5, 2015 at 1:12 PM, Demikhovsky, Elena
> <elena.demikhovsky at intel.com> wrote:
> > 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.
>
--
~Craig
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150205/772b3a3e/attachment.html>
More information about the llvm-commits
mailing list