> Hi Honza, > > Jan Hubicka wrote: > >But if you have something like > > > >a=__builtin_expect (b?1:0,0) > > > >and you produce > > > >a=b?predict_expr not_taken, 0,0 > >... > >if (a) > > unlikely path > > > >We need to check how it goes down to gimple. > > It seems as if something doesn't work in that case – at least I do > not understand the failure in > gfortran.dg/elemental_optional_args_5.f03. With the patch one has in > gfc_conv_procedure_call: > > + gfc_init_block (&err_block); > + gfc_add_expr_to_block (&err_block, > + build_predict_expr (PRED_ZERO, NOT_TAKEN)); > + gfc_add_expr_to_block (&err_block, > + fold_convert (TREE_TYPE (parmse.expr), > + null_pointer_node)); > > > tmp = fold_build2_loc (input_location, EQ_EXPR, boolean_type_node, > descriptor_data, > fold_convert (TREE_TYPE (descriptor_data), > null_pointer_node)); > parmse.expr > = fold_build3_loc (input_location, COND_EXPR, > + TREE_TYPE (parmse.expr), tmp, > + gfc_finish_block (&err_block), parmse.expr); > > > And for some reasons that will fail with -O0/-Os - it works with -O1 > or when one removes the PREDICT. > > Any idea what goes wrong here? I thought predictions can only > produce inefficient code but not wrong code.
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? > > > >So in short if we want the predictions to work reliably, we need to be sure > >we produce them in a way backend can reliably consume them. > > I fully concur. > > > 2) if there are the other cases (i.e. fortran language allows the failure > > flag to be stored into user variable and handled by user later), > > I will add internal use only argument to predict_expr and extend its > > handling. > > Again it won't be 100% reliable, since one conditional can then be > > handled > > by multiple predict_exprs that leads to generally unsolvable problem on > > how to combine probabilities. > > But hopefully those cases will mostly be simple - i.e. user will have > > if (failure_flag) > > make horrible death in controlled way > > statement somewhre after the allcation. > > We are smart enough to work out simple variants like > > if (failure_flag1 || failure_flag2) > > make horrible death in controlled way > > I think that's the case for, e.g. > ALLOCATE(variable, stat=status) > where gives status /= 0 in case an error occurs. Yep, in that case I will make the patch extending builtin_expect semantic then. > > > 3) make sure the runtime library calls are correctly annotated with > > noreturn/cold > > flags that may help back end to work out the cases it failed otherwise. > > I think that's already the case - and is the most important case. > > >>+/* Branch leading to an overflow are extremely unlikely. */ > >>+DEF_PREDICTOR (PRED_OVERFLOW, "overflow", PROB_VERY_LIKELY, > >>+ PRED_FLAG_FIRST_MATCH) > >I would even gor for PROB_ALWAYS here. > > Fine with me - I was only fearing that it would regard it as always > and remove the NOT_TAKEN path. But if it keeps it, using PROB_ALWAYS > makes sense. It is prediction only, not a fact one can use for optimization. Basically things that will never ever be on time critical path can be marked as PROB_ALWAYS. Here I think we can be pretty sure (well of course except for artifically constructed testcase. We can also make difference in between case where frontend autogenerate termination of the program and where user asks for status, perhaps?) > > >I would go for a bit more dscriptive names, like "fortran alloc > >overflow"/"fortran zero size alloc" etc. > >Those names will hopefuly be more obvious for middle end developer like > >myself. > > Okay. When the issue with elemental_optional_args_5.f03 is > understood, I will update the patch. > > Tobias
