What to do about pattern recognition not in .md order when the mode of a pattern operand is unspecified

[Hans-Peter Nilsson]

Other target-patches exposed this for me.
I have on the 4.7-branch an insn:

(jump_insn 245 277 246 (set (pc)
        (label_ref:SI 312)) whatever.c:511 -1
     (nil)
 -> 187)

and two (or more) pattern candidates in the following .md file
order:

(define_insn "jump"
  [(set (pc)
        (label_ref (match_operand 0 "" "")))]
  ""
  "ba %l0%#"
  [(set_attr "slottable" "has_slot")])

(define_insn "*indirect_jump_non_v32"
  [(set (pc) (match_operand:SI 0 "nonimmediate_operand" "rm"))]
  "!TARGET_V32"
  "jump %0")

As you know (or may want to dispute for some reason, so let's
settle that first), operands without specified modes on matching
patterns are wild-card matches; not VOIDmode matches or a kind
of lesser-order matches.

(ISTR some change, at least some discussion, that label_refs now
should have modes, but if the change happened, the documentation
wasn't sufficiently updated.  If so, the mode on the
"jump"-pattern above should be :SI, but when it's not specified,
it's a wild-card, so should still *match*.  It could also be
that the :SI is put there in error, but still the pattern
recognition order observation is valid.)

It seems that since 4.3, some change now causes the generated
pattern-matching tree in insn-recog.c:recog to try the pattern
*with the specified mode* before (eventually, seemingly last)
the one with the unspecified-mode label_ref.

What I'll do next depends.  Is there a reason to keep that
change (and document it) or should the bug just be fixed?

brgds, H-P

[Richard Sandiford-5]

With some trepidation, because I suspect I'm missing the point :-)

Hans-Peter Nilsson <hans-peter.nilsson@...> writes:

> Other target-patches exposed this for me.
> I have on the 4.7-branch an insn:
>
> (jump_insn 245 277 246 (set (pc)
>         (label_ref:SI 312)) whatever.c:511 -1
>      (nil)
>  -> 187)
>
> and two (or more) pattern candidates in the following .md file
> order:
>
> (define_insn "jump"
>   [(set (pc)
> (label_ref (match_operand 0 "" "")))]
>   ""
>   "ba %l0%#"
>   [(set_attr "slottable" "has_slot")])
>
> (define_insn "*indirect_jump_non_v32"
>   [(set (pc) (match_operand:SI 0 "nonimmediate_operand" "rm"))]
>   "!TARGET_V32"
>   "jump %0")

[...]
> It seems that since 4.3, some change now causes the generated
> pattern-matching tree in insn-recog.c:recog to try the pattern
> *with the specified mode* before (eventually, seemingly last)
> the one with the unspecified-mode label_ref.

This second one shouldn't match label_ref though, should it?
label_ref is an immediate_operand.

Was just wondering whether 4.3 was when the predicate codes
thing was added, so that genrecog knew that nonimmediate_operand
couldn't match.

Richard

[Hans-Peter Nilsson]

> From: Richard Sandiford <rdsandiford@...>
> Date: Sun, 22 Apr 2012 10:47:24 +0200

> With some trepidation, because I suspect I'm missing the point :-)

Maybe but maybe not.  Below it seems my observation was
misdiagnosed, and this is just a minor bug.

> Hans-Peter Nilsson <hans-peter.nilsson@...> writes:
> > Other target-patches exposed this for me.
> > I have on the 4.7-branch an insn:
> >
> > (jump_insn 245 277 246 (set (pc)
> >         (label_ref:SI 312)) whatever.c:511 -1
> >      (nil)
> >  -> 187)
> >
> > and two (or more) pattern candidates in the following .md file
> > order:
> >
> > (define_insn "jump"
> >   [(set (pc)
> > (label_ref (match_operand 0 "" "")))]
> >   ""
> >   "ba %l0%#"
> >   [(set_attr "slottable" "has_slot")])
> >
> > (define_insn "*indirect_jump_non_v32"
> >   [(set (pc) (match_operand:SI 0 "nonimmediate_operand" "rm"))]
> >   "!TARGET_V32"
> >   "jump %0")
> [...]
> > It seems that since 4.3, some change now causes the generated
> > pattern-matching tree in insn-recog.c:recog to try the pattern
> > *with the specified mode* before (eventually, seemingly last)
> > the one with the unspecified-mode label_ref.
>
> This second one shouldn't match label_ref though, should it?
> label_ref is an immediate_operand.

My bad, bad example, but I thought this was beside the point.
My observation was a predicate unexpectedly called, not a
pattern actually wrongly matched.

> Was just wondering whether 4.3 was when the predicate codes
> thing was added, so that genrecog knew that nonimmediate_operand
> couldn't match.

And that's the reason the generated code calls it? :]

BTW, there's a second almost-matching pattern in cris.md with
register_operand as the predicate that I didn't show, but just
for clarity of the generated code:

(define_insn "*indirect_jump_v32"
  [(set (pc) (match_operand:SI 0 "register_operand" "r"))]
  "TARGET_V32"
  "jump %0%#"
  [(set_attr "slottable" "has_slot")])

The generated code in insn-recog.c has, seeing PC in XEXP (x0, 0)
n a SET, arriving at L1691:

 L1691: ATTRIBUTE_UNUSED_LABEL
  x1 = XEXP (x0, 1);
  if (GET_MODE (x1) == SImode)
    goto L2792;
 L1687: ATTRIBUTE_UNUSED_LABEL
  switch (GET_CODE (x1))
    {
    case LABEL_REF:
      goto L1688;
    case IF_THEN_ELSE:
      goto L1701;
    default:
     break;
   }
  goto ret0;

 L2792: ATTRIBUTE_UNUSED_LABEL
  if (nonimmediate_operand (x1, SImode))
    {
      operands[0] = x1;
      goto L1692;
    }

 L2793: ATTRIBUTE_UNUSED_LABEL
  if (register_operand (x1, SImode))
    {
      operands[0] = x1;
      goto L1696;
    }
  goto L1687;

Note how the mode and predicates are tested before checking for
LABEL_REF.  But, as the predicates don't match, the last goto is
executed, going back to code checking for LABEL_REF.  Verified
by stepping in gdb.

Oh, I see what you mean; I changed the predicate of the first
pattern to general_operand and now there's a check for LABEL_REF
before testing for general_operand!  I also tried with a
target-local predicate, combinations both known and known not to
match LABEL_REF with and without specifying the mode.  What
seems to throw genrecog.c off, is when there's a mode specified
on the candidate with the predicate ("*indirect_jump_non_v32"
above); it then tests the mode and the known-to-fail predicates
before going back to the candidate with the bare label_ref.

So that leaves us just a much smaller problem with suboptimal
insn-recog.c code (predicates known to fail are still called).
I'll open a PR and point to this message.

BTW, the mode on the label_ref is not often seen: the label_ref
has to be modified by middle-end (calls to gen_rtx_LABEL_REF, in
this case by redirect_target IIRC) and not coming from the
"jump" expander.  Most targets still generated VOIDmode
LABEL_REFs in their jump patterns.

brgds, H-P