On Tue, May 21, 2024 at 11:36 PM David Malcolm <dmalc...@redhat.com> wrote: > > On Tue, 2024-05-21 at 15:13 +0000, Qing Zhao wrote: > > Thanks for the comments and suggestions. > > > > > On May 15, 2024, at 10:00, David Malcolm <dmalc...@redhat.com> > > > wrote: > > > > > > On Tue, 2024-05-14 at 15:08 +0200, Richard Biener wrote: > > > > On Mon, 13 May 2024, Qing Zhao wrote: > > > > > > > > > -Warray-bounds is an important option to enable linux kernal to > > > > > keep > > > > > the array out-of-bound errors out of the source tree. > > > > > > > > > > However, due to the false positive warnings reported in > > > > > PR109071 > > > > > (-Warray-bounds false positive warnings due to code duplication > > > > > from > > > > > jump threading), -Warray-bounds=1 cannot be added on by > > > > > default. > > > > > > > > > > Although it's impossible to elinimate all the false positive > > > > > warnings > > > > > from -Warray-bounds=1 (See PR104355 Misleading -Warray-bounds > > > > > documentation says "always out of bounds"), we should minimize > > > > > the > > > > > false positive warnings in -Warray-bounds=1. > > > > > > > > > > The root reason for the false positive warnings reported in > > > > > PR109071 is: > > > > > > > > > > When the thread jump optimization tries to reduce the # of > > > > > branches > > > > > inside the routine, sometimes it needs to duplicate the code > > > > > and > > > > > split into two conditional pathes. for example: > > > > > > > > > > The original code: > > > > > > > > > > void sparx5_set (int * ptr, struct nums * sg, int index) > > > > > { > > > > > if (index >= 4) > > > > > warn (); > > > > > *ptr = 0; > > > > > *val = sg->vals[index]; > > > > > if (index >= 4) > > > > > warn (); > > > > > *ptr = *val; > > > > > > > > > > return; > > > > > } > > > > > > > > > > With the thread jump, the above becomes: > > > > > > > > > > void sparx5_set (int * ptr, struct nums * sg, int index) > > > > > { > > > > > if (index >= 4) > > > > > { > > > > > warn (); > > > > > *ptr = 0; // Code duplications since "warn" does > > > > > return; > > > > > *val = sg->vals[index]; // same this line. > > > > > // In this path, since it's > > > > > under > > > > > the condition > > > > > // "index >= 4", the compiler > > > > > knows > > > > > the value > > > > > // of "index" is larger then 4, > > > > > therefore the > > > > > // out-of-bound warning. > > > > > warn (); > > > > > } > > > > > else > > > > > { > > > > > *ptr = 0; > > > > > *val = sg->vals[index]; > > > > > } > > > > > *ptr = *val; > > > > > return; > > > > > } > > > > > > > > > > We can see, after the thread jump optimization, the # of > > > > > branches > > > > > inside > > > > > the routine "sparx5_set" is reduced from 2 to 1, however, due > > > > > to > > > > > the > > > > > code duplication (which is needed for the correctness of the > > > > > code), > > > > > we > > > > > got a false positive out-of-bound warning. > > > > > > > > > > In order to eliminate such false positive out-of-bound warning, > > > > > > > > > > A. Add one more flag for GIMPLE: is_splitted. > > > > > B. During the thread jump optimization, when the basic blocks > > > > > are > > > > > duplicated, mark all the STMTs inside the original and > > > > > duplicated > > > > > basic blocks as "is_splitted"; > > > > > C. Inside the array bound checker, add the following new > > > > > heuristic: > > > > > > > > > > If > > > > > 1. the stmt is duplicated and splitted into two conditional > > > > > paths; > > > > > + 2. the warning level < 2; > > > > > + 3. the current block is not dominating the exit block > > > > > Then not report the warning. > > > > > > > > > > The false positive warnings are moved from -Warray-bounds=1 to > > > > > -Warray-bounds=2 now. > > > > > > > > > > Bootstrapped and regression tested on both x86 and aarch64. > > > > > adjusted > > > > > -Warray-bounds-61.c due to the false positive warnings. > > > > > > > > > > Let me know if you have any comments and suggestions. > > > > > > > > At the last Cauldron I talked with David Malcolm about these kind > > > > of > > > > issues and thought of instead of suppressing diagnostics to > > > > record > > > > how a block was duplicated. For jump threading my idea was to > > > > record > > > > the condition that was proved true when entering the path and do > > > > this > > > > by recording the corresponding locations > > > > Is only recording the location for the TRUE path enough? > > We might need to record the corresponding locations for both TRUE and > > FALSE paths since the VRP might be more accurate on both paths. > > Is only recording the location is enough? > > Do we need to record the pointer to the original condition stmt? > > Just to be clear: I don't plan to work on this myself (I have far too > much already to work on...); I'm assuming Richard Biener is > hoping/planning to implement this himself.
While I think some of this might be an improvement to those vast number of "false positive" diagnostics we have from (too) late diagnostic passes I do not have the cycles to work on this. Richard. > But feel free to ask me any questions about the diagnostic_path > machinery within the diagnostics subsystem. > > [...snip...] > > Regards > Dave >