Subzero: Add a peephole to fuse cmpxchg w/ later cmp+branch.

Add a peephole to fuse cmpxchg w/ later cmp+branch.

The cmpxchg instruction already sets ZF for comparing the return value
vs the expected value. So there is no need to compare eq again.

Lots of pexes-in-the-wild have this pattern. Some compare against
a constant, some compare against a variable.

BUG=https://code.google.com/p/nativeclient/issues/detail?id=3882
R=stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=c820ddf

[Jim Stichnoth, 2014-07-25 17:27:47]

https://codereview.chromium.org/413903002/diff/40001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/413903002/diff/40001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:2931: InstList::iterator I = Context.getCur();
Try to use LoweringContext instead of directly using InstList::iterator.  The
main thing is you get iteration while automatically ignoring deleted
instructions.

It may be that functionality would need to be added to LoweringContext. :)

https://codereview.chromium.org/413903002/diff/40001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:2938: // There might be phi assignments right
before the compare+branch, since this
I suggest also adding a note in the Phi lowering pass documenting places where
there may be assumptions on how it works, namely here and the scalar icmp/br
fusing code.

Phi lowering is on the list of things to be improved in Subzero.  One
possibility is to split edges and insert Phis along the edges.  (This would only
simplify code like this that tries to account for the assignments.)  Another
possibility is move the assignments around into other valid spots in the basic
block, which might change some other assumptions.

[jvoung (off chromium), 2014-07-28 22:55:26]

https://codereview.chromium.org/413903002/diff/40001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/413903002/diff/40001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:2931: InstList::iterator I = Context.getCur();
On 2014/07/25 17:27:47, stichnot wrote:
> Try to use LoweringContext instead of directly using InstList::iterator.  The
> main thing is you get iteration while automatically ignoring deleted
> instructions.
> 
> It may be that functionality would need to be added to LoweringContext. :)

Good point about the skipping deleted instructions. Done (at least used the
advance() method).

https://codereview.chromium.org/413903002/diff/40001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:2938: // There might be phi assignments right
before the compare+branch, since this
On 2014/07/25 17:27:46, stichnot wrote:
> I suggest also adding a note in the Phi lowering pass documenting places where
> there may be assumptions on how it works, namely here and the scalar icmp/br
> fusing code.
> 
> Phi lowering is on the list of things to be improved in Subzero.  One
> possibility is to split edges and insert Phis along the edges.  (This would
only
> simplify code like this that tries to account for the assignments.)  Another
> possibility is move the assignments around into other valid spots in the basic
> block, which might change some other assumptions.

Done (added to the existing comment). The splitting sounds nice to avoid these
assumptions. I assume the "finding other valid spots" is more for optimization
or perhaps for code randomization?

[Jim Stichnoth, 2014-07-29 18:08:45]

lgtm

https://codereview.chromium.org/413903002/diff/40001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/413903002/diff/40001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:2938: // There might be phi assignments right
before the compare+branch, since this
On 2014/07/28 22:55:26, jvoung wrote:
> On 2014/07/25 17:27:46, stichnot wrote:
> > I suggest also adding a note in the Phi lowering pass documenting places
where
> > there may be assumptions on how it works, namely here and the scalar icmp/br
> > fusing code.
> > 
> > Phi lowering is on the list of things to be improved in Subzero.  One
> > possibility is to split edges and insert Phis along the edges.  (This would
> only
> > simplify code like this that tries to account for the assignments.)  Another
> > possibility is move the assignments around into other valid spots in the
basic
> > block, which might change some other assumptions.
> 
> Done (added to the existing comment). The splitting sounds nice to avoid these
> assumptions. I assume the "finding other valid spots" is more for optimization
> or perhaps for code randomization?

I was thinking more about possible optimizations.  For example, for certain
kinds of cheap register allocators, we could try to move the Phi assignment
closer to the last use of the Phi source operand so that the source operand is
more likely to be available in a physical register.  But code randomization may
be a good idea.

[jvoung (off chromium), 2014-07-29 21:38:58]

Committed patchset #5 manually as rc820ddf (presubmit successful).