linux-sparse.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Christopher Li <sparse@chrisli.org>
To: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
Cc: Dibyendu Majumdar <mobile@majumdar.org.uk>,
	Linux-Sparse <linux-sparse@vger.kernel.org>,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: Simple SSA status
Date: Mon, 4 Sep 2017 14:29:46 -0400	[thread overview]
Message-ID: <CANeU7Q==zoxfgzGLKS00AqZ-M07Whr9Nu5gZtANd4PK_u3J47A@mail.gmail.com> (raw)
In-Reply-To: <20170903202629.bipsxi7xnmh3y3oy@ltop.local>

On Sun, Sep 3, 2017 at 4:26 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>>
>> Merge it when it is ready?
>
> I've put it on hold for the moment.

Thanks for the update. Glad to heard you back.

> The conversion of purely local vars was the easy job.
> The real challenge is to do it for the other loads & stores
> (in simplify_symbol_usage() & simplify_memops()).
> I'm working on it and have some encouraging results:
> - correct phi-nodes
> - pass the test-suite
> - no crashes in GCC test-suite and others
> - no infinite loops
> - no catastrophic performance on exceptional workoads
> - roughly convert as much stores & loads as currently
> - good performance (within 2-5% as rc5 on some workloads)
> - in some case, surprising good effect on optimization

That is great. Very exciting news.

>
> I don't know yet if keeping the Simple SSA during linearization
> will be worth to keep or not.

I have the same question as well.  When I find out the Simple SSA
can't handle memory to register using pointers. It already means
we need to keep the other non local variable path alive.
We can do some bench mark to find out. If the performance
is very similar, remove the simple SSA will actually simplify
the code because we only need to care about one code path.

>> Right now I do wish the SSSA has the two options I request
>> before.
>
> I don't remember what was the other option you asked but keeping
> both the old and the new method is not something I'm interested in.
> We know that the current method is broken. In fact, it's broken in two
> different ways:

I have no intend to keep the broken behavior. My wish list for the two
options rephrased:
1) Option to by pass the simpole SSA conversion which generate the
    raw load/store instruction before convert memory to register.
2) Option to do the memory to register conversion not cover by
    simple SSA conversion. It sounds like you implement this already.

> - the phi-nodes are mis-placed (they are on the use site instead of the
>   meet point).

Right. The meet point on the book also call Dominance Frontier :-)

> - each phi-nodes have as many sources as there are definitions for this
>   variable (more or less) instead of having one for each parents.

I am not sure I understand this. If you always place phi node at
DF. It will have as many source as incoming edge. That is part
of the common SSA dominance property. If we do it right, I can't
see why we still need the phi source instruction.


>
> I also recently discovered that the logic for aliases is broken.
> For example, code like:
>         int foo(void)
>         {
>                 int i = 1;
>                 int *a;
>                 int **p;
>
>                 a = &i;
>                 p = &a;
>                 *p[0] = 0;
>                 return i;
>         }
> is mis-compiled as it always returns 1 instead of 0
> (it fails to see that *p[0] is aliased to i).

That is exactly why we need instruction level aliases
analyze. The SSA conversion should be done after
aliases analyze.
>
> What I'm interested in is, in this order:
> 1) produce correct IR

Agree.

> 2) convert as much loads & stores as currently
>    (but only when it's correct to do it, of course)

Agree.

> 3) have performance that is similar as we currently have

Agree.

>
> Point 2) is needed to avoid more false warnings during
> context checking (and have a very significant effect on
> performance).

It seems we are all in agreement.

Chris

  reply	other threads:[~2017-09-04 18:29 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-21 19:39 master merge plans Christopher Li
2017-08-22 13:32 ` Luc Van Oostenryck
2017-08-22 14:47   ` Christopher Li
2017-08-22 15:51     ` Christopher Li
2017-08-22 20:05       ` Luc Van Oostenryck
2017-08-23 20:50         ` Christopher Li
2017-08-29 11:27           ` Christopher Li
2017-09-03 19:24             ` Luc Van Oostenryck
2017-08-22 14:53   ` Dibyendu Majumdar
2017-08-22 14:59     ` Christopher Li
2017-09-03 20:26       ` Simple SSA status Luc Van Oostenryck
2017-09-04 18:29         ` Christopher Li [this message]
2017-09-04 20:07           ` Luc Van Oostenryck
2017-09-04 20:37             ` Dibyendu Majumdar
2017-09-04 20:55               ` Luc Van Oostenryck
2017-09-04 21:24                 ` Dibyendu Majumdar
2017-09-04 23:31             ` Christopher Li
2017-09-05  0:55               ` Luc Van Oostenryck
2017-09-05  3:28                 ` Christopher Li
2017-09-07  2:03                   ` Luc Van Oostenryck
2017-09-07  2:15                     ` Linus Torvalds
2017-09-07  2:55                       ` Christopher Li
2017-09-07  3:17                         ` Luc Van Oostenryck
2017-09-07  4:04                           ` Christopher Li
2017-09-07  4:49                             ` Luc Van Oostenryck
2017-09-07  6:17                               ` Christopher Li
2017-09-07  7:38                                 ` Luc Van Oostenryck
2017-09-07  3:05                       ` Luc Van Oostenryck
2017-09-07  2:20                     ` Christopher Li
2017-09-07  3:46                       ` Luc Van Oostenryck
2017-09-07  4:02                         ` Christopher Li
2017-09-07  4:24                           ` Luc Van Oostenryck
2017-09-07  4:33                             ` Christopher Li

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CANeU7Q==zoxfgzGLKS00AqZ-M07Whr9Nu5gZtANd4PK_u3J47A@mail.gmail.com' \
    --to=sparse@chrisli.org \
    --cc=linux-sparse@vger.kernel.org \
    --cc=luc.vanoostenryck@gmail.com \
    --cc=mobile@majumdar.org.uk \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).