llvm dev - Dec 2016 - Non-determinism in LLVM codegen

On Tue, Dec 13, 2016 at 4:57 PM, Grang, Mandeep Singh via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> Everyone,
>
> The following patch to reverse iterate SmallPtrSet's has now been
merged:
> https://reviews.llvm.org/D26718
>
> This is how LLVM behavior will change due to this patch:
> - In LLVM builds with assertions enabled, SmallPtrSet's would always be
> reverse iterated by default.
>   This default behavior can be overridden via the flag "-mllvm
> -reverse-iterate=<true/false>".
>
> - In LLVM builds with assertions disabled, SmallPtrSet's would continue
to
> be always forward iterated.
>   This behavior cannot be overridden as the above flag is available only in
> builds with assertions enabled.
>
> Currently, the following unit tests are failing in Release+Asserts mode due
> to the difference in SmallPtrSet iteration order:
>     Clang :: Analysis/keychainAPI.m
>     Clang :: Analysis/malloc.c
>     Clang :: Rewriter/objc-modern-metadata-visibility.mm
>     Clang :: SemaCXX/warn-loop-analysis.cpp
>     LLVM :: Transforms/SimplifyCFG/bug-25299.ll
Wait, so you committed a patch that breaks these tests? Should you fix
them first?

IIUC, LLVM_ABI_BREAKING_CHECKS is WITH_ASSERTS by default, so your
change, which is known to break things, is enabled for those of us
enabling asserts?

Or am I misunderstanding?

On Tue, Dec 13, 2016 at 6:39 PM, Hans Wennborg <hans at chromium.org>
wrote:
> On Tue, Dec 13, 2016 at 4:57 PM, Grang, Mandeep Singh via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
>> Everyone,
>>
>> The following patch to reverse iterate SmallPtrSet's has now been
merged:
>> https://reviews.llvm.org/D26718
>>
>> This is how LLVM behavior will change due to this patch:
>> - In LLVM builds with assertions enabled, SmallPtrSet's would
always be
>> reverse iterated by default.
>>   This default behavior can be overridden via the flag "-mllvm
>> -reverse-iterate=<true/false>".
>>
>> - In LLVM builds with assertions disabled, SmallPtrSet's would
continue to
>> be always forward iterated.
>>   This behavior cannot be overridden as the above flag is available
only in
>> builds with assertions enabled.
>>
>> Currently, the following unit tests are failing in Release+Asserts mode
due
>> to the difference in SmallPtrSet iteration order:
>>     Clang :: Analysis/keychainAPI.m
>>     Clang :: Analysis/malloc.c
>>     Clang :: Rewriter/objc-modern-metadata-visibility.mm
>>     Clang :: SemaCXX/warn-loop-analysis.cpp
>>     LLVM :: Transforms/SimplifyCFG/bug-25299.ll
>
> Wait, so you committed a patch that breaks these tests? Should you fix
> them first?
>
> IIUC, LLVM_ABI_BREAKING_CHECKS is WITH_ASSERTS by default, so your
> change, which is known to break things, is enabled for those of us
> enabling asserts?
>
> Or am I misunderstanding?
Looks like Mehdi reverted already.

> -----Original Message-----
> From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of
Hans
> Wennborg via llvm-dev
> Sent: Tuesday, December 13, 2016 6:42 PM
> To: Grang, Mandeep Singh
> Cc: llvm-dev at lists.llvm.org; Nico Weber
> Subject: Re: [llvm-dev] Non-determinism in LLVM codegen
> 
> On Tue, Dec 13, 2016 at 6:39 PM, Hans Wennborg <hans at chromium.org>
wrote:
> > On Tue, Dec 13, 2016 at 4:57 PM, Grang, Mandeep Singh via llvm-dev
> > <llvm-dev at lists.llvm.org> wrote:
> >> Everyone,
> >>
> >> The following patch to reverse iterate SmallPtrSet's has now
been
> merged:
> >> https://reviews.llvm.org/D26718
> >>
> >> This is how LLVM behavior will change due to this patch:
> >> - In LLVM builds with assertions enabled, SmallPtrSet's would
always be
> >> reverse iterated by default.
> >>   This default behavior can be overridden via the flag
"-mllvm
> >> -reverse-iterate=<true/false>".
> >>
> >> - In LLVM builds with assertions disabled, SmallPtrSet's would
continue
> to
> >> be always forward iterated.
> >>   This behavior cannot be overridden as the above flag is
available
> only in
> >> builds with assertions enabled.
> >>
> >> Currently, the following unit tests are failing in Release+Asserts
mode
> due
> >> to the difference in SmallPtrSet iteration order:
> >>     Clang :: Analysis/keychainAPI.m
> >>     Clang :: Analysis/malloc.c
> >>     Clang :: Rewriter/objc-modern-metadata-visibility.mm
> >>     Clang :: SemaCXX/warn-loop-analysis.cpp
> >>     LLVM :: Transforms/SimplifyCFG/bug-25299.ll
> >
> > Wait, so you committed a patch that breaks these tests? Should you fix
> > them first?
> >
> > IIUC, LLVM_ABI_BREAKING_CHECKS is WITH_ASSERTS by default, so your
> > change, which is known to break things, is enabled for those of us
> > enabling asserts?
> >
> > Or am I misunderstanding?
> 
> Looks like Mehdi reverted already.
Disabled by default, not reverted, but yes turned it off.
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev