This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ] Implement conditional returns.
ClosedPublic

Authored by koriakin on Feb 17 2016, 9:28 AM.

Details

Summary

Return is now considered a predicable instruction, and is converted
to a newly-added CondReturn (which maps to BCR to %r14) instruction by
the if conversion pass.

Also, fused compare-and-branch transform knows about conditional
returns, emitting the proper fused instructions for them.

This transform triggers on a *lot* of tests, hence the huge diffstat.
The changes are mostly jX to br %r14 -> bXr %r14.

Diff Detail

Repository
rL LLVM

Event Timeline

koriakin retitled this revision from to [SystemZ] Implement conditional returns..
koriakin updated this object.
koriakin added a reviewer: uweigand.
koriakin added a subscriber: llvm-commits.
uweigand edited edge metadata.Feb 17 2016, 2:28 PM

Your code changes look good in general. However, I agree that some of resulting transformations are less than optimal.

The if conversion happens before fused compares and BRCT are generated, thus winning over them. Could be OK for the fused compares, I suppose, but BRCT should be better than cond return. I suppose BRCT generation could be changed to cover that case, thoiugh. I've added junk instructions before return in the CIJ* and BRCT testcases to make sure they are still emitted.

For the fused compares, I think we should be able to get the best of both worlds by first generating the conditional return, and then fusing *that* with a compare into one of the C(G)RB or C(G)IB instructions (which LLVM currently doesn't use at all).

For BRCT, I suspect the problem is a special case of the strange loop transformation you mention later. If the backwards branch of the loop weren't modified by IfConversion, then the BRCT should still be detected.

Now as to that loop scenario, if I understand it correctly, it seems IfConversion recognizes the backwards branch of a loop at the very end of the function as an instance of the (inverted) ICSimple pattern. This seems questionable. I'm wondering whether this can be fixed in common code somehow (but I'm not sure if there's loop structure information available at this point in the process).

Failing that, I guess the backend's isProfitableToDupForIfCvt callback might try to detect that scenario via a low BranchProbability parameter being passed in (assuming that common code assigns a high probability to the backwards branch of a loop by default).

koriakin updated this object.
koriakin edited edge metadata.
koriakin set the repository for this revision to rL LLVM.

New version. Fused compare-and-branch instructions are emitted for conditional returns now, and the loop problem of the previous version is avoided based on branch probabilities. I'm now somewhat convinced that's the right way to do it (as opposed to looking at the loop structure) - for a loop with low loop probability (high return probability), like the compare-and-swap retries in atomicrmw-minmax-0[34].ll tests, the transform is actually still a win.

uweigand accepted this revision.Apr 7 2016, 9:13 AM
uweigand edited edge metadata.

LGTM, thanks! I'll check it in shortly ...

This revision is now accepted and ready to land.Apr 7 2016, 9:13 AM
This revision was automatically updated to reflect the committed changes.