Luo, Yuanke via llvm-dev
2021-Apr-15 14:07 UTC
[llvm-dev] [RFC] [X86] Emit unaligned vector moves on avx machine with option control.
Yes, replacing aligned move instruction with unaligned move instruction doesn’t solve all the issue that happens in optimization pipeline, but it doesn’t make things worse. One advantage for unaligned move is that it makes the behavior the same no matter the mov instruction is folded or not. Do you think it is worth to support this feature if compiler can help users avoid changing their complex legacy code? Thanks Yuanke From: James Y Knight <jyknight at google.com> Sent: Thursday, April 15, 2021 9:09 PM To: Liu, Chen3 <chen3.liu at intel.com> Cc: llvm-dev at lists.llvm.org; Luo, Yuanke <yuanke.luo at intel.com>; Maslov, Sergey V <sergey.v.maslov at intel.com> Subject: Re: [llvm-dev] [RFC] [X86] Emit unaligned vector moves on avx machine with option control. On Thu, Apr 15, 2021 at 4:43 AM Liu, Chen3 <chen3.liu at intel.com<mailto:chen3.liu at intel.com>> wrote: Hi, James Y Knight. I'm not sure if you misunderstood this patch. This patch won’t change any alignment information in IR and MI, which means ‘load…align 32’ will always keep the alignment information but select ‘vmovups’ instead of ‘vmovaps’ during ISEL. It can be simply considered that the only thing this patch does is to replace the aligned-move mnemonic with the unaligned-move mnemonic (in fact, we shouldn’t call it replace but emit unaligned). I think there is no impact on optimization or code layout. Yes -- I understood that, and that is exactly why this patch is not OK. Giving LLVM incorrect information about the alignment of objects causes problems other than just the emission of movaps instructions -- that alignment information is correct gets relied upon throughout the optimization pipeline. So, a command-line option to "fix" only that one instruction is not something which we can reasonably provide, because it will not reliably fix users' problems. A program which is being "mis"-compiled due to the use of misaligned objects might still be miscompiled by LLVM when using your proposed patch. ("mis" in quotes, since the compiler is correctly compiling the code according to the standard, even if not according to the user's expectations). The second paragraph of my original email describes an alternative patch that you could write, which would reliably fix such miscompilation -- effectively creating a variant of C where creating and accessing misaligned objects has fully defined behavior. (And, just to reiterate, my initial feeling is that creating such an option is not a worthwhile endeavor, but I could be persuaded otherwise.) After discussion, we think this option more like changing the behavior when process with unaligned memory: raising exception or accepting performance degradation. Maybe the option is more like “no-exception-on-unalginedmem”. We do have some users want this feature. They can accept “run slow” but do not want exception. Thanks. Chen Liu. From: Philip Reames <listmail at philipreames.com<mailto:listmail at philipreames.com>> Sent: Thursday, April 15, 2021 6:44 AM To: James Y Knight <jyknight at google.com<mailto:jyknight at google.com>>; Liu, Chen3 <chen3.liu at intel.com<mailto:chen3.liu at intel.com>> Cc: llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>; Luo, Yuanke <yuanke.luo at intel.com<mailto:yuanke.luo at intel.com>>; Maslov, Sergey V <sergey.v.maslov at intel.com<mailto:sergey.v.maslov at intel.com>> Subject: Re: [llvm-dev] [RFC] [X86] Emit unaligned vector moves on avx machine with option control. +1 to what James said. My reaction to the original proposal is a strong -1, and James did a good job of explaining why. Philip On 4/14/21 11:57 AM, James Y Knight via llvm-dev wrote: This is not a principled change -- it avoids a problem arising from one use of alignment information, but there are other uses of alignment in LLVM, and those will still cause problems, potentially less clearly. So, I think that this will not be a useful option to provide to users, in this form. What I suspect you actually want here is an option to tell Clang not to infer load/store alignments based on object types or alignment attributes -- instead treating everything as being potentially aligned to 1 unless the allocation is seen (e.g. global/local variables). Clang would still need to use the usual alignment computation for variable definitions and structure layout, but not memory operations. If clang emits "load ... align 1" instructions in LLVM IR, the right thing would then happen in the X86 backend automatically. My initial inclination is that this feature is also not particularly worthwhile to implement, but I'm open to being convinced that this is indeed valuable enough to be worthwhile. It should actually work reliably, and is somewhat in line with other such "not-quite-C" flags we provide (e.g. -fno-delete-null-pointer-checks). Of course, even with such an implementation, you can still have a problem with user code depending on alignof() returning a reliable answer (e.g., llvm::PointerUnion). Not much can be done about that. On Wed, Apr 14, 2021 at 2:07 PM Liu, Chen3 via llvm-dev <llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote: Hi all. We want to make a patch to always emit unaligned vector move instructions on AVX machine with option control. We do this for the following reason: 1. With AVX the performance for aligned vector move and unaligned vector move on X86 are the same if the address is aligned. In this case we prefer to use unaligned move because it can avoid some run time exceptions; 2. This fixes an inconsistency in optimization: suppose a load operation was merged into another instruction (e.g., load and add becomes `add [memop]'). If a misaligned pointer is passed to the two-instruction sequence, it will raise an exception. If the same pointer is passed to the memop instruction, it will work. Thus, the behavior of misalignment depends upon what optimization levels and passes are applied, and small source changes could cause issues to appear and disappear. It's better for the user to consistently use unaligned load/store to improve the debug experience; 1. Makes good use of HW that is capable of handling misaligned data gracefully. It is not necessarily a bug in users code but a third-part library. For example it would allow using a library built in old ages where stack alignment was 4-byte only. 2. Compatible with ICC so that users can easily use llvm; Roman Lebedev is worried that this patch will hide UB. In our opinions, UB doesn't have to mean raise an exception. The example code(https://godbolt.org/z/43bYPraoa) does have UB behavior but it is still valid (and reasonable) to interpret that UB as `go slower', instead of `raise exception'. Besides, as default we still emit aligned instructions as before, but we provide an option for users with this need. We have two patches discussing this issue, one of which has been abandoned: https://reviews.llvm.org/D88396 (abandoned) https://reviews.llvm.org/D99565 (in review) Thanks. Chen Liu. _______________________________________________ LLVM Developers mailing list llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev _______________________________________________ LLVM Developers mailing list llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210415/cf22baa3/attachment.html>
Philip Reames via llvm-dev
2021-Apr-15 15:36 UTC
[llvm-dev] [RFC] [X86] Emit unaligned vector moves on avx machine with option control.
IMO, no. We should encourage sanitizers instead. From experience, any code base where porting trips across this probably also has a bunch of other undefined behavior which is causing less obvious miscompiles, and also need found and fixed. That's why we have sanitizers. Philip On 4/15/21 7:07 AM, Luo, Yuanke via llvm-dev wrote:> > Yes, replacing aligned move instruction with unaligned move > instruction doesn’t solve all the issue that happens in optimization > pipeline, but it doesn’t make things worse. One advantage for > unaligned move is that it makes the behavior the same no matter the > mov instruction is folded or not. Do you think it is worth to support > this feature if compiler can help users avoid changing their complex > legacy code? > > Thanks > > Yuanke > > *From:* James Y Knight <jyknight at google.com> > *Sent:* Thursday, April 15, 2021 9:09 PM > *To:* Liu, Chen3 <chen3.liu at intel.com> > *Cc:* llvm-dev at lists.llvm.org; Luo, Yuanke <yuanke.luo at intel.com>; > Maslov, Sergey V <sergey.v.maslov at intel.com> > *Subject:* Re: [llvm-dev] [RFC] [X86] Emit unaligned vector moves on > avx machine with option control. > > On Thu, Apr 15, 2021 at 4:43 AM Liu, Chen3 <chen3.liu at intel.com > <mailto:chen3.liu at intel.com>> wrote: > > Hi, James Y Knight. > > I'm not sure if you misunderstood this patch. This patch won’t > change any alignment information in IR and MI, which means > ‘load…align 32’ will always keep the alignment information but > select ‘vmovups’ instead of ‘vmovaps’ during ISEL. It can be > simply considered that the only thing this patch does is to > replace the aligned-move mnemonic with the unaligned-move mnemonic > (in fact, we shouldn’t call it replace but emit unaligned). I > think there is no impact on optimization or code layout. > > Yes -- I understood that, and that is exactly why this patch is not > OK. Giving LLVM incorrect information about the alignment of > objects causes problems other than just the emission of movaps > instructions -- that alignment information is correct gets relied upon > throughout the optimization pipeline. > > So, a command-line option to "fix" only that one instruction is not > something which we can reasonably provide, because it will not > reliably fix users' problems. A program which is being "mis"-compiled > due to the use of misaligned objects might still be miscompiled by > LLVM when using your proposed patch. ("mis" in quotes, since the > compiler is correctly compiling the code according to the standard, > even if not according to the user's expectations). > > The second paragraph of my original email describes an alternative > patch that you could write, which /would/ reliably fix such > miscompilation -- effectively creating a variant of C where creating > and accessing misaligned objects has fully defined behavior. (And, > just to reiterate, my initial feeling is that creating such an option > is not a worthwhile endeavor, but I could be persuaded otherwise.) > > After discussion, we think this option more like changing the > behavior when process with unaligned memory: raising exception or > accepting performance degradation. Maybe the option is more like > “no-exception-on-unalginedmem”. We do have some users want this > feature. They can accept “run slow” but do not want exception. > > Thanks. > > Chen Liu. > > *From:* Philip Reames <listmail at philipreames.com > <mailto:listmail at philipreames.com>> > *Sent:* Thursday, April 15, 2021 6:44 AM > *To:* James Y Knight <jyknight at google.com > <mailto:jyknight at google.com>>; Liu, Chen3 <chen3.liu at intel.com > <mailto:chen3.liu at intel.com>> > *Cc:* llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>; > Luo, Yuanke <yuanke.luo at intel.com <mailto:yuanke.luo at intel.com>>; > Maslov, Sergey V <sergey.v.maslov at intel.com > <mailto:sergey.v.maslov at intel.com>> > *Subject:* Re: [llvm-dev] [RFC] [X86] Emit unaligned vector moves > on avx machine with option control. > > +1 to what James said. My reaction to the original proposal is a > strong -1, and James did a good job of explaining why. > > Philip > > On 4/14/21 11:57 AM, James Y Knight via llvm-dev wrote: > > This is not a principled change -- it avoids a problem arising > from /one/ use of alignment information, but there are other > uses of alignment in LLVM, and those will still cause > problems, potentially less clearly. So, I think that this will > not be a useful option to provide to users, in this form. > > What I suspect you /actually/ want here is an option to tell > Clang not to infer load/store alignments based on object types > or alignment attributes -- instead treating everything as > being potentially aligned to 1 unless the allocation is seen > (e.g. global/local variables). Clang would still need to use > the usual alignment computation for variable definitions and > structure layout, but not memory operations. If clang emits > "load ... align 1" instructions in LLVM IR, the right thing > would then happen in the X86 backend automatically. > > My initial inclination is that this feature is also not > particularly worthwhile to implement, but I'm open to being > convinced that this is indeed valuable enough to be > worthwhile. It should actually work reliably, and is somewhat > in line with other such "not-quite-C" flags we provide (e.g. > -fno-delete-null-pointer-checks). Of course, even with such an > implementation, you can still have a problem with user code > depending on alignof() returning a reliable answer (e.g., > llvm::PointerUnion). Not much can be done about that. > > On Wed, Apr 14, 2021 at 2:07 PM Liu, Chen3 via llvm-dev > <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote: > > Hi all. > > We want to make a patch to always emit unaligned vector > move instructions on AVX machine with option control. We > do this for the following reason: > > 1. With AVX the performance for aligned vector move and > unaligned vector move on X86 are the same if the > address is aligned. In this case we prefer to use > unaligned move because it can avoid some run time > exceptions; > 2. This fixes an inconsistency in optimization: suppose a > load operation was merged into another instruction > (e.g., load and add becomes `add [memop]'). If a > misaligned pointer is passed to the two-instruction > sequence, it will > > raise an exception. If the same pointer is passed to the > memop instruction, it will work. Thus, the behavior of > misalignment depends upon what optimization levels and > passes are applied, and small source changes could cause > > issues to appear and disappear. It's better for the user > to consistently use unaligned load/store to improve the > debug experience; > > 3. Makes good use of HW that is capable of handling > misaligned data gracefully. It is not necessarily a > bug in users code but a third-part library. For > example it would allow using a library built in old > ages where stack alignment was 4-byte only. > 4. Compatible with ICC so that users can easily use llvm; > > Roman Lebedev is worried that this patch will hide UB. In > our opinions, UB doesn't have to mean raise an exception. > The example code(https://godbolt.org/z/43bYPraoa > <https://godbolt.org/z/43bYPraoa>) does have UB behavior > but it is still valid (and reasonable) to interpret that > UB as `go slower', > > instead of `raise exception'. Besides, as default we still > emit aligned instructions as before, but we provide an > option for users with this need. > > We have two patches discussing this issue, one of which > has been abandoned: > > https://reviews.llvm.org/D88396 > <https://reviews.llvm.org/D88396> (abandoned) > > https://reviews.llvm.org/D99565 > <https://reviews.llvm.org/D99565> (in review) > > Thanks. > > Chen Liu. > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > <https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev> > > _______________________________________________ > > LLVM Developers mailing list > > llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org> > > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev <https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev> > > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210415/63a70683/attachment-0001.html>
via llvm-dev
2021-Apr-15 15:54 UTC
[llvm-dev] [RFC] [X86] Emit unaligned vector moves on avx machine with option control.
I’ve debated whether to chime in, and decided it can’t hurt. Sony had to do a similar downstream patch for PS4. Our use-case is pretty constrained, though. There’s only one toolchain, there’s only one target chip, so we don’t have any portability considerations to think about. What we do have are games shipping on DVD that can’t be re-released and can’t even necessarily be patched, and a strict backward compatibility requirement. So, if there’s a game out there that didn’t happen to follow all the alignment requirements, and it worked on console version 1.00, it still has to be working on version 100.00. (FTR, we’re currently on about 8.00.) I don’t think we ever seriously considered upstreaming our patch. The circumstances where it’s really necessary do exist, but are pretty limited. I don’t think arguments of the form “it’s okay because X Y and Z” are going to be persuasive. “We have this situation in the following circumstances” might help people understand. --paulr From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of Luo, Yuanke via llvm-dev Sent: Thursday, April 15, 2021 10:07 AM To: James Y Knight <jyknight at google.com>; Liu, Chen3 <chen3.liu at intel.com> Cc: llvm-dev at lists.llvm.org; Maslov, Sergey V <sergey.v.maslov at intel.com> Subject: Re: [llvm-dev] [RFC] [X86] Emit unaligned vector moves on avx machine with option control. Yes, replacing aligned move instruction with unaligned move instruction doesn’t solve all the issue that happens in optimization pipeline, but it doesn’t make things worse. One advantage for unaligned move is that it makes the behavior the same no matter the mov instruction is folded or not. Do you think it is worth to support this feature if compiler can help users avoid changing their complex legacy code? Thanks Yuanke -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210415/d7dda98c/attachment.html>
James Y Knight via llvm-dev
2021-Apr-15 16:23 UTC
[llvm-dev] [RFC] [X86] Emit unaligned vector moves on avx machine with option control.
I believe strongly that we should not add an option which makes it sound like it makes unaligned access work, when we know for a fact that optimization passes make use of the alignment information and will also break such misaligned-object-using code. Worse, we also can predict that even more such optimizations will be added in future versions of llvm, and break such code more. Offering such an option which seems like it would do what they want, but which doesn't actually, is a perfect recipe for creating unhappy users. That's why I've been saying over and over that *if* we do end up providing some "make unaligned access work" option, it needs to make it *actually work*, reliably, both now and in the future. On Thu, Apr 15, 2021 at 10:07 AM Luo, Yuanke <yuanke.luo at intel.com> wrote:> Yes, replacing aligned move instruction with unaligned move instruction > doesn’t solve all the issue that happens in optimization pipeline, but it > doesn’t make things worse. One advantage for unaligned move is that it > makes the behavior the same no matter the mov instruction is folded or not. > Do you think it is worth to support this feature if compiler can help users > avoid changing their complex legacy code? > > > > Thanks > > Yuanke > > > > *From:* James Y Knight <jyknight at google.com> > *Sent:* Thursday, April 15, 2021 9:09 PM > *To:* Liu, Chen3 <chen3.liu at intel.com> > *Cc:* llvm-dev at lists.llvm.org; Luo, Yuanke <yuanke.luo at intel.com>; > Maslov, Sergey V <sergey.v.maslov at intel.com> > *Subject:* Re: [llvm-dev] [RFC] [X86] Emit unaligned vector moves on avx > machine with option control. > > > > > > > > On Thu, Apr 15, 2021 at 4:43 AM Liu, Chen3 <chen3.liu at intel.com> wrote: > > Hi, James Y Knight. > > > > I'm not sure if you misunderstood this patch. This patch won’t change any > alignment information in IR and MI, which means ‘load…align 32’ will always > keep the alignment information but select ‘vmovups’ instead of ‘vmovaps’ > during ISEL. It can be simply considered that the only thing this patch > does is to replace the aligned-move mnemonic with the unaligned-move > mnemonic (in fact, we shouldn’t call it replace but emit unaligned). I > think there is no impact on optimization or code layout. > > > > Yes -- I understood that, and that is exactly why this patch is not OK. > Giving LLVM incorrect information about the alignment of objects causes > problems other than just the emission of movaps instructions -- that > alignment information is correct gets relied upon throughout the > optimization pipeline. > > > > So, a command-line option to "fix" only that one instruction is not > something which we can reasonably provide, because it will not reliably fix > users' problems. A program which is being "mis"-compiled due to the use of > misaligned objects might still be miscompiled by LLVM when using your > proposed patch. ("mis" in quotes, since the compiler is correctly compiling > the code according to the standard, even if not according to the user's > expectations). > > > > The second paragraph of my original email describes an alternative patch > that you could write, which *would* reliably fix such miscompilation -- > effectively creating a variant of C where creating and accessing misaligned > objects has fully defined behavior. (And, just to reiterate, my initial > feeling is that creating such an option is not a worthwhile endeavor, but I > could be persuaded otherwise.) > > > > After discussion, we think this option more like changing the behavior > when process with unaligned memory: raising exception or accepting > performance degradation. Maybe the option is more like > “no-exception-on-unalginedmem”. We do have some users want this feature. > They can accept “run slow” but do not want exception. > > > > Thanks. > > Chen Liu. > > > > > > *From:* Philip Reames <listmail at philipreames.com> > *Sent:* Thursday, April 15, 2021 6:44 AM > *To:* James Y Knight <jyknight at google.com>; Liu, Chen3 < > chen3.liu at intel.com> > *Cc:* llvm-dev at lists.llvm.org; Luo, Yuanke <yuanke.luo at intel.com>; > Maslov, Sergey V <sergey.v.maslov at intel.com> > *Subject:* Re: [llvm-dev] [RFC] [X86] Emit unaligned vector moves on avx > machine with option control. > > > > +1 to what James said. My reaction to the original proposal is a strong > -1, and James did a good job of explaining why. > > Philip > > On 4/14/21 11:57 AM, James Y Knight via llvm-dev wrote: > > This is not a principled change -- it avoids a problem arising from *one* use > of alignment information, but there are other uses of alignment in LLVM, > and those will still cause problems, potentially less clearly. So, I think > that this will not be a useful option to provide to users, in this form. > > > > What I suspect you *actually* want here is an option to tell Clang not to > infer load/store alignments based on object types or alignment attributes > -- instead treating everything as being potentially aligned to 1 unless the > allocation is seen (e.g. global/local variables). Clang would still need to > use the usual alignment computation for variable definitions and structure > layout, but not memory operations. If clang emits "load ... align 1" > instructions in LLVM IR, the right thing would then happen in the X86 > backend automatically. > > > > My initial inclination is that this feature is also not > particularly worthwhile to implement, but I'm open to being convinced that > this is indeed valuable enough to be worthwhile. It should actually work > reliably, and is somewhat in line with other such "not-quite-C" flags we > provide (e.g. -fno-delete-null-pointer-checks). Of course, even with such > an implementation, you can still have a problem with user code depending on > alignof() returning a reliable answer (e.g., llvm::PointerUnion). Not much > can be done about that. > > > > > > On Wed, Apr 14, 2021 at 2:07 PM Liu, Chen3 via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > > Hi all. > > > > We want to make a patch to always emit unaligned vector move instructions > on AVX machine with option control. We do this for the following reason: > > > > 1. With AVX the performance for aligned vector move and unaligned > vector move on X86 are the same if the address is aligned. In this case we > prefer to use unaligned move because it can avoid some run time exceptions; > 2. This fixes an inconsistency in optimization: suppose a load > operation was merged into another instruction (e.g., load and add becomes > `add [memop]'). If a misaligned pointer is passed to the two-instruction > sequence, it will > > raise an exception. If the same pointer is passed to the memop > instruction, it will work. Thus, the behavior of misalignment depends upon > what optimization levels and passes are applied, and small source changes > could cause > > issues to appear and disappear. It's better for the user to consistently > use unaligned load/store to improve the debug experience; > > 1. Makes good use of HW that is capable of handling misaligned data > gracefully. It is not necessarily a bug in users code but a third-part > library. For example it would allow using a library built in old ages where > stack alignment was 4-byte only. > 2. Compatible with ICC so that users can easily use llvm; > > > > Roman Lebedev is worried that this patch will hide UB. In our opinions, UB > doesn't have to mean raise an exception. The example code( > https://godbolt.org/z/43bYPraoa) does have UB behavior but it is still > valid (and reasonable) to interpret that UB as `go slower', > > instead of `raise exception'. Besides, as default we still emit aligned > instructions as before, but we provide an option for users with this need. > > > > We have two patches discussing this issue, one of which has been abandoned: > > https://reviews.llvm.org/D88396 (abandoned) > > https://reviews.llvm.org/D99565 (in review) > > > > Thanks. > > Chen Liu. > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > > > > _______________________________________________ > > LLVM Developers mailing list > > llvm-dev at lists.llvm.org > > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210415/f887ca1c/attachment.html>