Re: [RFA] [tree-optimization/80576] Handle non-constant sizes in DSE

[Richard Biener <richard dot guenther at gmail dot com>]

On Fri, Aug 16, 2019 at 8:15 PM Jeff Law <law@redhat.com> wrote:
>
> On 8/16/19 12:09 PM, Marc Glisse wrote:
> > On Fri, 16 Aug 2019, Jeff Law wrote:
> >
> >> This patch improves our ability to detect dead stores by handling cases
> >> where the size memcpy, memset, strncpy, etc call is not constant.  This
> >> addresses some, but not all, of the issues in 80576.
> >>
> >> The key here is when the size is not constant we can make conservative
> >> decisions that still give us a chance to analyze the code for dead
> >> stores.
> >>
> >> Remember that for dead store elimination, we're trying to prove that
> >> given two stores, the second store overwrites (partially or fully) the
> >> same memory locations as the first store.  That makes the first store
> >> either partially or fully dead.
> >>
> >> When we encounter the first store, we set up a bitmap of bytes written
> >> by that store (live_bytes).  We then look at subsequent stores and clear
> >> the appropriate entries in the bitmap.
> >>
> >> If the first store has a nonconstant length argument we can use the
> >> range of the length argument (max) and the size of the destination
> >> object to make a conservative estimation of how many bytes are written.
> >>
> >> For the second store the conservative thing to do for a non-constant
> >> length is to use the minimum of the range of the length argument.
> >
> > So I guess it won't handle things like
> >
> > void f(char*p,int n){
> >   __builtin_memset(p,3,n);
> >   __builtin_memset(p,7,n);
> > }
> >
> > where we know nothing about the length, except that it is the same? Or
> > do you look at symbolic ranges?
> Nope.  I think ao_ref can represent that, so it'd just be a matter of
> recording "n" as the length, then verifying that the second call's
> length is "n" as well.  That makes the first call dead.  We'd have to
> bypass the byte tracking in that case, but I think that's trivial
> because we already have a means to do that when the sizes are too large.
>
> >
> >> This doesn't come up a lot in practice.  But it also happens to put some
> >> of the infrastructure in place to handle strcpy and strcpy_chk which are
> >> needed to fully resolve 80576.
> >>
> >> Bootstrapped and regression tested on x86, x86_64, ppc64le, ppc64,
> >> ppc32, aarch64, sparc, s390x and probably others.  Also verified that
> >> the tests work on the various *-elf targets in my tester.
> >>
> >> OK for the trunk?
> >
> > ENOPATCH
> Opps.    Attached.

+static tree
+objsize_from_type (tree object)
+{
+  if (TREE_CODE (object) != ADDR_EXPR)
+    return NULL_TREE;
+
+  tree type = TREE_TYPE (object);
+  if (POINTER_TYPE_P (type))

that if looks suspicious...  I'd say
  if (!POINTER_TYPE_P (type))
    return NULL_TREE;

is better

+    type = TREE_TYPE (type);

+  if (TREE_CODE (type) == ARRAY_TYPE && !array_at_struct_end_p (object))
+    {

array_at_struct_end_p will never return true here since you pass it
an ADDR_EXPR...  you wanted to pass TREE_OPERAND (object, 0) here?

Also you seem to use this info to constrain optimization when you
might remember that types of addresses do not carry such information...
Thus it should be "trivially" possible to write a testcase that is miscompiled
after your patch.  I also don't see this really exercised in the
testcases you add?
All of them reference a static array a ...

+           if (TREE_CODE (size) != INTEGER_CST)

TREE_CODE (size) == SSA_NAME you want

+             {
+               wide_int minbound, maxbound;
+               value_range_kind rng = get_range_info (size,
&minbound, &maxbound);
+               if (rng == VR_RANGE)

Richard.

>
> Jeff
>
>