[Fortran] RFC / RFA patch for using build_predict_expr instead of builtin_expect / PR 58721

[Tobias Burnus]

Pre-remark:

I had hoped that something like the following patch would work. However, 
it will lead to a bunch of run-time segfaults in the test suite - but 
the original dump seems to be fine and I also fail to spot the problem 
when looking at the patch. Thus, instead of posting a working patch, I 
have below a nonworking draft patch. Questions:

(a) Does the build_predict_expr() usage look in principle right - or do 
I do something obviously wrong?

(b) Is it possible to use build_predict_expr() for code like "a = (cond) 
? (expr1) : (expr2)"? After gimplifying, there should be a BB [and a 
phi] - thus, adding the predict to the BB should be possible. But is it 
also possible on tree level with code like above? That mainly applies to 
trans-array.c but also in another case.

(c) How to handle:  if (cond1 || overflow_cond) {...}? Currently, 
"overflow_cond" is marked as unlikely but as build_predict_expr() 
applies to the branch/basic block and not to the condition, how should 
it be handled?

(d) And in general: Does the approach look right and the choice of PRED_ 
and their values?

* * *

gfortran uses internally builtin_expect [i.e. gfc_likely()/gfc_unlikely()].

On the other hand, builtin_expect is also used by (C/C++) users. Google 
did some tests and empirical result was that unlikely is more likely 
than GCC's prediction handling expected. I believe, the likelyhood for 
likely() reduced from a hit rate of 99 to 90.

For gfortran, the change leads to PR58721, where some code is regarded 
as less likely as before and to no inlining. In many cases, the 
unlikely() is not necessary as those branches have a call to an error 
function, annotated with "noreturn". As explicit user request overrides 
other heurisitic, the builtin_expect will cause that the predictor 
regards the gfc_unlikely() as *more* likely than without (as "noreturn" 
is less likely).


This patch tries to address this for the different cases:

a) If there is a "noreturn" function called, the gfc_(un)likely has been 
removed. [Unless, I wrongly classified is in one of the next items; 
however, a "noreturn" will override another PRED_*. Thus, it shouldn't 
matter.]

b) As OVERFLOWs should only occur extremely rarely, I used PRED_OVERFLOW 
with PROB_VERY_LIKELY (i.e. the branch is very likely to be NOT_TAKEN).

c) In many cases, GCC permits to replace run-time aborts for failures by 
asking for a status, e.g. "DEALLOCATE(var, stat=status)"; I have used 
PRED_FAIL with HITRATE(80) for those - but one can argue about the value.

d) gfortran supports a run-time warning. That's currently only used for 
copy-in/out. I think both branches can be approximately equally likely 
(with no-warning being more common in most codes). However, the warning 
is only shown once. I have set PRED_WARN_CALL's hitrate to  to 70 - but 
that's also an arbitrary value.

e) As in the trans_image_index: There one had:  "if (cond || 
invalid_bound) {... } else {...}". The "invalid_bound" is marked as 
unlikely but PRED_* cannot be use as only part of the branch condition 
is unlikely. [I've removed the unlikely.]


Does the patch look sensible? Does the addition of the PRED_* make 
sense? How about their values?


Tobias
-------------- next part --------------
A non-text attachment was scrubbed...
Name: predict.diff
Type: text/x-patch
Size: 18673 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20131210/bb33daa7/attachment.bin>