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

[Tobias Burnus]

Pre-remark: I currently get Stage 2/Stage 3 miscompares. As this is 
unlikely to be caused by my patch, it properly means that Honza's patch 
bitrotted even though it still applies.


Dear all,

attached is an updated patch set for this PR.

Background: gfortran uses internally __builtin_expect. However, it 
recently changed from very likely (99%, if I recall correctly) to 90% 
(adaptable via a "--param"). As side-effect, some conditions for error 
paths in gfortran became more likely - even if they lead to no-return 
code as PRED_NORETURN is overridden by an explicit __builtin_expect 
(alias PRED_BUILTIN_EXPECT).

I have attached two patches:

a) Honza's patch, which he attached to the PR. (without changelog, 
rediffed - which essentially means that I have taken out the failing 
part, which seems to be already applied.)
b) An updated patch of mine.

Honza's patch permits to add, optionally, a PRED_* value to builtin_predict.

My patch adds a bunch of PRED_* for Fortran, which should cover all 
existing use of builtin_expect; I have added a PRED_ for every different 
use I found. The PRED_* numbers I use are hopefully good guesses but I 
am open for suggestions for their numbers. Additional, one can argue 
whether the PRED_* names are well chosen or whether there are too many 
PRED_.

On the Fortran side, I have replaced the builtin_expect by either 
build_predict_expr (which is added to the (basic) block which is 
(un)likely) and by the three-argument version of builtin_expect as we 
often use it like "(cont1 || builtin_expect(...))" which isn't 
representable with build_predict_expr. (However, I might have used 
build_predict_expr in some additional cases.)

The patch was attempted to be bootstrapped on x86-64-gnu-linux.

Tobias


Am 15.12.2013 23:15, schrieb Jan Hubicka:
>> Jan Hubicka wrote:
>>> Yep, they should not roduce incorrect code. Isn't the problem
>>> that you expect the whole expression to have value and predit_expr
>>> "reutrns" nothing?
>>> Can you check what lands into gimple?
>> That could well be the case – I replace "0" by "{ built_predict; 0
>> }" and I wouldn't be surprised if the built_predict causes problem
> Yep, though this is also the case whrere you really want to predict
> a value rather than code path, so the extended bultin_expect is probably
> only resonable approach here.
>
> I am not really generic person, but perhaps it is a difference in between {
> built_predict; 0 } that is stmt with no value and built_predict,0 that is an
> expression with value?
>
> I will look into the builtin_expect extension. Then we can probably
> keem gfc_likely/unlikely with extra argument specifying the predictor
> for cases like this and use predict_expr in cases where you
> really produce runtime conditional like
> if (...)
>    { bulitin_predict; abort ();}
>
> Honza
>> as it returns 'nothing'. At least the basic block belonging to
>> "else" (<D.1934>) is empty:
>>
>> Original dump (4.8 and 4.9 with predict patch):
>>            sub1 (&v[S.0 + -1], (logical(kind=4)) __builtin_expect
>> ((integer(kind=8)) (D.1897 == 0B), 0) ? 0B : &(*D.1897)[(S.0 +
>> D.1901) * D.1903 + D.1898]);
>> and
>>            sub1 (&v[S.0 + -1], D.1917 != 0B ? &(*D.1917)[(S.0 +
>> D.1921) * D.1923 + D.1918] : (void) 0);
>>
>> Gimple of 4.8:
>>        if (D.1916 != 0) goto <D.1917>; else goto <D.1918>;
>>        <D.1917>:
>>        iftmp.2 = 0B;
>>        goto <D.1919>;
>>        <D.1918>:
>> ...
>>        iftmp.2 = &*D.1897[D.1922];
>>        <D.1919>:
>> ..
>>        sub1 (D.1924, iftmp.2);
>>
>> gimple of 4.9 with patch:
>>        if (D.1917 != 0B) goto <D.1933>; else goto <D.1934>;
>>        <D.1933>:
>>        D.1935 = S.0 + D.1921;
>>        D.1936 = D.1935 * D.1923;
>>        D.1937 = D.1936 + D.1918;
>>        iftmp.2 = &*D.1917[D.1937];
>>        goto <D.1938>;
>>        <D.1934>:
>>        <D.1938>:
>>        D.1939 = S.0 + -1;
>>        D.1940 = &v[D.1939];
>>        sub1 (D.1940, iftmp.2);
>>
>> Tobias
>>
>> PS: That's for the code:
>>
>>    implicit none
>>    type t2
>>      integer, allocatable :: a
>>      integer, pointer :: p2(:) => null()
>>    end type t2
>>    type(t2), save :: x
>>    integer, save :: s, v(2)
>>    call sub1 (v, x%p2)
>> contains
>>    elemental subroutine sub1 (x, y)
>>      integer, intent(inout) :: x
>>      integer, intent(in), optional :: y
>>    end subroutine sub1
>> end

-------------- next part --------------
A non-text attachment was scrubbed...
Name: builtin-expect-3arg.diff
Type: text/x-patch
Size: 7535 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20140302/fd3bce1d/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fortran-builtin-expect-3arg.diff
Type: text/x-patch
Size: 23261 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20140302/fd3bce1d/attachment-0001.bin>