[PATCH] D22305: [BasicAA] Strip phi nodes, when all incoming values are the same.

Tue Aug 9 09:26:40 PDT 2016

I guess it clear, but to be sure, the numbers are percentage of the compile
time slow down after applying the patch:  ((After - Before) / Before)

From:	Ehsan A Amiri/Toronto/IBM
To:	Daniel Berlin <dberlin at dberlin.org>
Cc:	Hal Finkel <hfinkel at anl.gov>, llvm-commits
            <llvm-commits at lists.llvm.org>, reviews+D22305+public
            +92ca108e50bc4651 at reviews.llvm.org
Date:	08/09/2016 12:24 PM
Subject:	Re: [PATCH] D22305: [BasicAA] Strip phi nodes, when all
            incoming values are the same.

I have measured the impact of throwing away cache (Daniel's patch) on the
compile time. I used a subset of CPU 2006 benchmarks. I only compiled C/C++
files, for the benchmarks that had some fortran files.

This is the result: There are two significant slow downs: namd (16%) and
h264ref (9%).  for the rest please see bottom of the email.

So between the following two, I believe first we want to give a try to (1)
before measuring the impact of (2). Is that correct?

(1) removing only clobber entries from the cache.
(2) fixing BasicAA to work better with phi-s with a unique incoming value.

For (1) I do not have any intuition into how much it will save. I know for
local cache we cache the result when there is no dependency found. I don't
remember how the global cache works.

astar       1.6
bzip2       3.5
cactus      3.2
calculix    2.5
gcc         4.6
gobmk       1.5
gromacs     4.3
hmmer       3.3
mcf         1.3
perlbench   3.2
povray      3.1
soplex      1.9

From:	Daniel Berlin <dberlin at dberlin.org>
To:	Ehsan A Amiri/Toronto/IBM at IBMCA
Cc:	Hal Finkel <hfinkel at anl.gov>, llvm-commits
            <llvm-commits at lists.llvm.org>, reviews+D22305+public
            +92ca108e50bc4651 at reviews.llvm.org
Date:	07/15/2016 05:22 PM
Subject:	Re: [PATCH] D22305: [BasicAA] Strip phi nodes, when all
            incoming values are the same.

On Fri, Jul 15, 2016 at 1:34 PM, Ehsan A Amiri <amehsan at ca.ibm.com> wrote:
  So I will first experiment with (3). If we don't like the outcome, then
  we can experiment with (2) to see how feasible is that.

  Daniel: Thanks for the response to my question. But now I need to ask two
  more question to understand things:

  As mentioned, in your testcase, there is another bug here.  Remember
  that GVN iterates until things stop changing. So *the first gvn pass*
  should have never left the IR with that redundant PHI node in the first
  place.

  1- IIUC, we are saying that at the end of one GVN iteration, the IR
  should satisfy some properties. Is it possible to describe this
  properties?

Close. I mean "at the end of executing a  GVN pass, the IR should satisfy
some properties" (iterations are internal to gvn, and in the current GVN,
it may take multiple iterations to satisfy these properties).

The properties it should satisfy are[1]

1. Every value (V) dominated by another provably equivalent value (Leader)
should have V replaced by Leader.  This is "equivalence without taking into
account conditionals", and not even complete equivalence, but "equivalence
that this GVN algorithm can detect".  The GVN algorithm used is not a
complete one, so it misses plenty of provably equivalences.

As a trivial example:

for (int i =0, j = 0; i < 50; ++i, ++j)
{
int x = i + 2;
int y = j + 2;
A[x] = y
int z = A[y];
}

i, j, x, y, z are all equivalent.
Current GVN will detect none of these. This is not fixable.

NewGVN is not complete, but it will detect all of these.
Complete algorithms will detect all that can be detected :)

2. Every instruction should be simplified as done by SimplifyInst. If
simplification produces a value, all uses should be replaced with that
simplified value

[1] There are PRE and conditional propagation properties, but they are
harder to describe.

Now, for *each iteration*, there are invariants. The above is an iteration
level invariant except for the case where it does PRE and inserts code.  In
the case it does PRE and inserts code, it may not meet the above
conditions.

  2- I suspect that the "other bug" that you are talking about is a GVN
  bug. Could you elaborate on that if this is correct?

Yes.

  IIRC from my debugging, the redundant phi node was created and deleted in
  iteration 0 of GVN.

 I do not see this happen in some cases where i modify your testcase.
Again i was not being precise enough, and meant more "things your testcase
exposes after debugging the code".

  It was just that cache issue that happened between the creation and
  deletion of redundant phi node that caused this....

  Thanks
  Ehsan

  Inactive hide details for Daniel Berlin ---07/15/2016 04:08:45 PM---> >
  Daniel Berlin ---07/15/2016 04:08:45 PM---> >

  From: Daniel Berlin <dberlin at dberlin.org>
  To: Hal Finkel <hfinkel at anl.gov>
  Cc: llvm-commits <llvm-commits at lists.llvm.org>, reviews+D22305+public
  +92ca108e50bc4651 at reviews.llvm.org, Ehsan A Amiri/Toronto/IBM at IBMCA
  Date: 07/15/2016 04:08 PM
  Subject: Re: [PATCH] D22305: [BasicAA] Strip phi nodes, when all incoming
  values are the same.

        Or do we need (2) because in general this kind of phi node is seen
        in many programs? Or something else that I do not see....

        That is hal's view.

        My view was that we should measure the impact of the change and
        then decide; but that if we have a use case for it, I'd rather it
        be inside the generic utility than a specific work-around in
        BasicAA.

  Yes, sorry, was writing quickly, did not mean to imply anything else ;)

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160809/97530113/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graycol.gif
Type: image/gif
Size: 105 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160809/97530113/attachment.gif>