On Fri, May 29, 2020 at 02:43:12PM +0200, Richard Biener wrote: > So I tried to understand the circumstances the rs6000 patterns FAIL > but FAILed ;) It looks like some outs of rs6000_emit_vector_cond_expr > are unwarranted and the following should work: > > diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c > index 8435bc15d72..5503215a00a 100644 > --- a/gcc/config/rs6000/rs6000.c > +++ b/gcc/config/rs6000/rs6000.c > @@ -14638,8 +14638,7 @@ rs6000_emit_vector_compare (enum rtx_code rcode,
(Different function, btw) > rtx mask2; > > rev_code = reverse_condition_maybe_unordered (rcode); > - if (rev_code == UNKNOWN) > - return NULL_RTX; > + gcc_assert (rev_code != UNKNOWN); reverse_condition_maybe_unordered is documented as possibly returning UNKNOWN. The current implementation doesn't, sure. But fix that first? rs6000_emit_vector_compare can fail for several other reasons, too -- including when rs6000_emit_vector_compare_inner fails. > @@ -14737,8 +14736,7 @@ rs6000_emit_vector_cond_expr (rtx dest, rtx > op_true, rtx op_false, > rtx cond2; > bool invert_move = false; > > - if (VECTOR_UNIT_NONE_P (dest_mode)) > - return 0; > + gcc_assert (VECTOR_UNIT_NONE_P (dest_mode)); Why can this condition never be true? (Missing a ! btw) It needs a big comment if you want to make wide assumptions like that, in any case. Pretty much *all* (non-trivial) asserts need an explanation. (And perhaps VECTOR_UNIT_ALTIVEC_OR_VSX_P is better). > /* Get the vector mask for the given relational operations. */ > mask = rs6000_emit_vector_compare (rcode, cc_op0, cc_op1, mask_mode); > > if (!mask) > return 0; > > fail but that function recurses heavily - from reading > rs6000_emit_vector_compare_inner > it looks like power can do a lot of compares but floating-point LT which > reverse_condition_maybe_unordered would turn into UNGE which is not > handled either. > But then rs6000_emit_vector_compare just tries GT for that anyway (not UNGE) > so > it is actually be handled (but should not?). > > So I bet the expansion of the patterns cannot fail at the moment. Thus I'd > replace the FAIL with a gcc_unreachable () and see if we have test > coverage for those > FAILs. I am not comfortable with that at all. > Segher - do you actually know this code to guess why the patterns are > defensive? Yes. If you want to change the documented semantics of widely used functions, please propose that? Segher
