RE: fix PR46029: reimplement if conversion of loads and stores

[Abe <abe_skolnik at yahoo dot com>]

> (if-conversion could directly generate masked load/stores
>  of course and not use a scratch-pad at all in that case).

IMO that`s a great idea, but I don`t know how to do it.  Hints would be welcome.  In particular, how does one "generate masked load/stores" at the GIMPLE level?


> But are we correctly handling these cases in the current if conversion code?

I`m uncertain to what that is intended to refer, but I believe Sebastian would agree that the new if converter is safer than the old one in terms of correctness at the time of running the code being compiled.


> Abe's changes would seem like a step forward from a correctness standpoint

Not to argue, but as a point of humility: Sebastian did by far the most work on this patch.  I just modernized it and helped move it along.  Even _that_ was done with Sebastian`s help.


> even if they take us a step backwards from a performance standpoint.

For now, we have a few performance regressions, and so far we have found that it`s non-trivial to remove all of those regressions.
We may be better off pushing the current patch to trunk and having the performance regressions currently introduced by the new if converter be fixed by later patches.
Pushing to trunk gives us excellent "visibility" amongst GCC hackers, so the code will get more "eyeballs" than if it lingers in an uncommitted patch or in a branch.
I, for one, would love some help in fixing these performance regressions. ;-)

If fixing the performance regressions winds up taking too long, perhaps the current imperfect patch could be undone on trunk just before a release is tagged,
and then we`ll push it in again when trunk goes back to being allowed to be unstable?  According to my analysis of the data near the end of the page at
<https://gcc.gnu.org/develop.html>, we have until roughly April of 2016 to work on not-yet-perfect patches in trunk.


> > So the question is whether we get more non-vectorized if-converted
> > code out of this (and thus whether we want to use --param
> > allow-store-data-races to get the old code back which is nicer to less
> > capable CPUs and probably faster than using scatter/gather or masked loads/stores).

> I do think conditionalizing some of this on the allow-store-data-races makes sense.

I think having both the old if-converter and the new one "live on" in GCC is nontrivial, but not impossible.  I also don`t think it`s the best long-term goal,
but only a short-term workaround.  In the long run, IMO there should be only one if converter, albeit perhaps with tweaking flags [e.g. "-fallow-unsafe-if-conversion"].


> I also wonder if we should really care about load data races (not sure your patch does).

According to a recent long discussion I had with Sebastian, our current patch does not have the flaw I was concerned it might have in terms of loads because:

  [1] the scratchpad is only being used to if-convert assignments to thread-local scalars, never to globals/statics, and because...

  [2] the gimplifier is supposed to detect "the address of this scalar has been taken" and when such is detected in the code being compiled,
      it causes the scalar to no longer look like a scalar in GIMPLE so that we are also safe from stale-data problems that could come from
      corner-case code that takes the address of a thread-local variable and gives that address to another thread [which then proceeds to
      overwrite the value in the supposedly-thread-local scalar that belongs to a different thread from the one doing the writing]


Regards,

Abe