[PATCH] D74155: [X86CmovConversion] Make heuristic for optimized cmov depth more conservative (PR44539)

Fri Feb 7 04:42:59 PST 2020

andreadb added a comment.

In D74155#1862443 <https://reviews.llvm.org/D74155#1862443>, @lebedev.ri wrote:

> Okay. Then this needs tests.
>  There's a lot of bugs referenced in https://bugs.llvm.org/show_bug.cgi?id=44539,
>  at least one of those should be affected, right?

The problem is that it is unclear how to write a good reliable test for this issue.

That is because there is no way currently to tell if a branch obtained from the conversion of a CMOV is going to be predictable or not. The cmov conversion only models whether a conversion is profitable under the assumption of a reasonably well-predicted branch. There will always be cases where the lack of knowledge about branch probabilities will negatively affect the work done by this pass.

Nikita's patch is at least trying to make the cmov cost heuristic a bit more conservative, and I see it as a good short-term workaround. We know that this is not meant to be a proper fix in practice. But it is enough to fix all the known regressions in libgav1 (and it doesn't regress the rates of any SPEC CPU 2017 benchmark).

Even with this patch, cmov conversion is still performed in libgav1 ( function `@_ZN7libgav13dsp12_GLOBAL__N_112CdefFilter_CILi8EhEEvPKvliiiiiiiiiiPvl`) on three CMOVs. It just so happen (by luck) that those three expanded CMOVs are not the problematic (i.e. unpredictable) ones.

Before this patch:

  ===-------------------------------------------------------------------------===
                            ... Statistics Collected ...
  ===-------------------------------------------------------------------------===

  48 x86-cmov-conversion - Number of CMOV-group candidates
   1 x86-cmov-conversion - Number of CMOV-conversion profitable loops
  11 x86-cmov-conversion - Number of optimized CMOV-groups
   2 x86-cmov-conversion - Number of unsupported CMOV-groups

After this patch:

  ===-------------------------------------------------------------------------===
                            ... Statistics Collected ...
  ===-------------------------------------------------------------------------===

  48 x86-cmov-conversion - Number of CMOV-group candidates
   1 x86-cmov-conversion - Number of CMOV-conversion profitable loops
   3 x86-cmov-conversion - Number of optimized CMOV-groups
   2 x86-cmov-conversion - Number of unsupported CMOV-groups

We could write a test that checks for example the statistics related to the number of expanded CMOVs for a function. However, that test would be extremely fragile, and therefore not very good. So I personally don't particularly mind if this change doesn't come with a test case.

That being said, I don't have a strong opinion on this. If the absence of a test case is too controversial, then there is always the less controversial option of fully disabling this pass for now.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74155/new/

https://reviews.llvm.org/D74155