Page MenuHomePhabricator

[X86] Improved X86::CMOV to Branch heuristic

Authored by aaboud on Jul 31 2017, 3:40 AM.



Resolved PR33954.
This patch contains two more constraint that aim to reduce the noise cases where we convert CMOV into branch for small gain, and spending more cycles due to overhead.

Diff Detail


Event Timeline

aaboud created this revision.Jul 31 2017, 3:40 AM
RKSimon edited edge metadata.Jul 31 2017, 4:10 AM

Is there any way that you can get test cases from PR33954?

aaboud updated this revision to Diff 108946.Jul 31 2017, 10:04 AM

Added couple of tests that represents the bug in PR33954 and cover the change in the patch.
Notice that I needed to modify one of the previous tests.
I measured the performance manually on a small manually written main function, and noticed improvement with the new decision - as explained in the LIT test pr33954.ll.

Is there any way that you can get test cases from PR33954?

Hi Simon,
Are the below tests good enough?
Do you have other comments?


zvi added inline comments.Aug 2 2017, 12:21 PM
409 ↗(On Diff #108946)

Would this be a bit more readable?

if (diff[i] < GainCycleThreashold)
  return false;

bool WorthOptLoop = false;
if (Diff[1] == Diff[0])
    WorthOptLoop = Diff[0] * 8 >= LoopDepth[0].Depth;
else if (Diff[1] > Diff[0])
    WorthOptLoop =
        (Diff[1] - Diff[0]) * 2 >= (LoopDepth[1].Depth - LoopDepth[0].Depth) &&
        (Diff[1] * 8 >= LoopDepth[1].Depth);
1 ↗(On Diff #108946)

Since the -x86-cmov-converter is invoked explicitly, should the -x86-cmov-converter-threshold=4 flag be added too?

29 ↗(On Diff #108946)

Maybe emphasize that in this case we bail out due to a low absolute gain in *cycles*, despite the *relative* gain being acceptable.

99 ↗(On Diff #108946)

if (t *a> b)

aaboud updated this revision to Diff 109450.Aug 2 2017, 4:08 PM
aaboud marked an inline comment as done.

Addressed Zvi comments.

aaboud added a comment.Aug 2 2017, 4:10 PM

Thanks Zvi for the comments.
Please see answers below.

1 ↗(On Diff #108946)

Actually, I want to guard the threshold, whoever change it, should revisit this test.
However, this test is not guarding against turning off the CMOV optimization., it is just verifying that the optimization itself works as expected.

99 ↗(On Diff #108946)

Why is this important?

This revision is now accepted and ready to land.Aug 3 2017, 7:23 AM
This revision was automatically updated to reflect the committed changes.