On 10/21/21 3:59 PM, Andrew Pinski wrote:
On Thu, Oct 21, 2021 at 8:04 AM Andrew MacLeod <amacl...@redhat.com> wrote:
On 10/19/21 7:13 PM, Andrew Pinski wrote:
On Tue, Oct 19, 2021 at 3:32 PM Andrew MacLeod <amacl...@redhat.com> wrote:
On 10/19/21 5:13 PM, Andrew Pinski wrote:
On Tue, Oct 19, 2021 at 1:29 PM Andrew MacLeod via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
using testcase ifcvt-4.c:
typedef int word __attribute__((mode(word)));
word
foo (word x, word y, word a)
{
word i = x;
word j = y;
/* Try to make taking the branch likely. */
__builtin_expect (x > y, 1);
if (x > y)
{
i = a;
j = i;
}
return i * j;
The testcase is broken anyways.
The builtin_expect should be inside the if to have any effect. Look
at the estimated values:
if (x_3(D) > y_4(D))
goto <bb 4>; [50.00%] <<-- has been reversed.
else
goto <bb 3>; [50.00%]
;; succ: 4 [50.0% (guessed)] count:536870912 (estimated
locally) (TRUE_VALUE,EXECUTABLE)
;; 3 [50.0% (guessed)] count:536870912 (estimated
locally) (FALSE_VALUE,EXECUTABLE)
See how it is 50/50?
The testcase is not even testing what it says it is testing. Just
happened to work previously does not mean anything. Move the
builtin_expect inside the if and try again. I am shocked it took this
long to find the testcase issue really.
Thanks,
Andrew Pinski
Moving the expect around doesn't change anything, in fact, it makes it
worse since fre and evrp immediately eliminate it as true if it is in
the THEN block.
I think you misunderstood the change I was saying to do.
Try this:
typedef int word __attribute__((mode(word)));
word
foo (word x, word y, word a)
{
word i = x;
word j = y;
/* Try to make taking the branch likely. */
if (__builtin_expect (x > y, 1))
{
i = a;
j = i;
}
return i * j;
}
/* { dg-final { scan-rtl-dump "2 true changes made" "ce1" } } */
This should fix the "estimated values" to be more correct.
Thanks,
Andrew Pinski
estimated values are now correct, but it changes nothing. I think you
are right that it could be an artifact of going in and out of ASSERT_EXPRs.
;; basic block 2, loop depth 0, count 1073741824 (estimated locally),
maybe hot
;; prev block 0, next block 5, flags: (NEW, REACHABLE, VISITED)
;; pred: ENTRY [always] count:1073741824 (estimated locally)
(FALLTHRU,EXECUTABLE)
if (x_6(D) > y_7(D))
goto <bb 3>; [90.00%]
else
goto <bb 5>; [10.00%]
;; succ: 3 [90.0% (guessed)] count:966367640 (estimated
locally) (TRUE_VALUE,EXECUTABLE)
;; 5 [10.0% (guessed)] count:107374184 (estimated
locally) (FALSE_VALUE,EXECUTABLE)
;; basic block 5, loop depth 0, count 107374184 (estimated locally),
maybe hot
;; prev block 2, next block 3, flags: (NEW)
;; pred: 2 [10.0% (guessed)] count:107374184 (estimated
locally) (FALSE_VALUE,EXECUTABLE)
x_2 = ASSERT_EXPR <x_6(D), x_6(D) <= y_7(D)>;
y_3 = ASSERT_EXPR <y_7(D), y_7(D) >= x_2>;
goto <bb 4>; [100.00%]
;; succ: 4 [always] count:107374184 (estimated locally) (FALLTHRU)
;; basic block 3, loop depth 0, count 966367640 (estimated locally),
maybe hot
;; prev block 5, next block 4, flags: (NEW, REACHABLE, VISITED)
;; pred: 2 [90.0% (guessed)] count:966367640 (estimated
locally) (TRUE_VALUE,EXECUTABLE)
x_1 = ASSERT_EXPR <x_6(D), x_6(D) > y_7(D)>;
y_12 = ASSERT_EXPR <y_7(D), y_7(D) < x_1>;
;; succ: 4 [always] count:966367640 (estimated locally)
(FALLTHRU,EXECUTABLE)
;; basic block 4, loop depth 0, count 1073741824 (estimated locally),
maybe hot
;; prev block 3, next block 1, flags: (NEW, REACHABLE, VISITED)
;; pred: 5 [always] count:107374184 (estimated locally) (FALLTHRU)
;; 3 [always] count:966367640 (estimated locally)
(FALLTHRU,EXECUTABLE)
# i_4 = PHI <x_2(5), a_8(D)(3)>
# j_5 = PHI <y_3(5), a_8(D)(3)>
_9 = i_4 * j_5;
# VUSE <.MEM_10(D)>
return _9;
nothing is done, and upon removing the ASSERTs, it becomes:
;; basic block 2, loop depth 0, count 1073741824 (estimated locally),
maybe hot
;; prev block 0, next block 3, flags: (NEW, REACHABLE, VISITED)
;; pred: ENTRY [always] count:1073741824 (estimated locally)
(FALLTHRU,EXECUTABLE)
if (x_6(D) > y_7(D))
goto <bb 4>; [90.00%]
else
goto <bb 3>; [10.00%]
;; succ: 4 [90.0% (guessed)] count:966367640 (estimated
locally) (TRUE_VALUE,EXECUTABLE)
;; 3 [10.0% (guessed)] count:107374184 (estimated
locally) (FALSE_VALUE,EXECUTABLE)
;; basic block 3, loop depth 0, count 107374184 (estimated locally),
maybe hot
;; prev block 2, next block 4, flags: (NEW, VISITED)
;; pred: 2 [10.0% (guessed)] count:107374184 (estimated
locally) (FALSE_VALUE,EXECUTABLE)
;; succ: 4 [always] count:107374184 (estimated locally)
(FALLTHRU,EXECUTABLE)
;; basic block 4, loop depth 0, count 1073741824 (estimated locally),
maybe hot
;; prev block 3, next block 1, flags: (NEW, REACHABLE, VISITED)
;; pred: 3 [always] count:107374184 (estimated locally)
(FALLTHRU,EXECUTABLE)
;; 2 [90.0% (guessed)] count:966367640 (estimated
locally) (TRUE_VALUE,EXECUTABLE)
# i_4 = PHI <x_6(D)(3), a_8(D)(2)>
# j_5 = PHI <y_7(D)(3), a_8(D)(2)>
_9 = i_4 * j_5;
# VUSE <.MEM_10(D)>
return _9;
And it appears that the testcase is checking for a situation that only
happens because VRP2 happens to lay it out this way.
In fact, if you simply turn off VRP2 the testcase will again fail.
Even funnier/sadder... if I make the change you suggest to the
testcase... it then fails with current trunk... So that change actually
disables what VRP2 happens to do :-P .
So the testcase seems problematic and fragile.
ha, even sadder, if I *reverse* what you provided and make it:
if (__builtin_expect (x > y, 0)),
THEN everything works as expected, and VRP becomes irrelevant. it works
under all circumstance I can find. ( This seems to be the behaviour it
is looking for :-P)
What do we think about reverseing that test case? like so:
Andrew
diff --git a/gcc/testsuite/gcc.dg/ifcvt-4.c b/gcc/testsuite/gcc.dg/ifcvt-4.c
index ec142cfd943..e74e449b402 100644
--- a/gcc/testsuite/gcc.dg/ifcvt-4.c
+++ b/gcc/testsuite/gcc.dg/ifcvt-4.c
@@ -13,8 +13,7 @@ foo (word x, word y, word a)
word i = x;
word j = y;
/* Try to make taking the branch likely. */
- __builtin_expect (x > y, 1);
- if (x > y)
+ if (__builtin_expect (x > y, 0))
{
i = a;
j = i;
