Re: [FORTRAN PATCH] Reduce gfc_match_strings usage (part 2)

[Tobias Schlüter <tobias dot schlueter at physik dot uni-muenchen dot de>]

Hi Roger,

Roger Sayle wrote:
> On Sun, March 11, 2007 9:02 am, Tobias Schlüter wrote:
> > As an aside to all the discussion about the implementation strategy for
> > the matchers, I'd like to point out that standard Fortran has a .xor.
> > operator, which is called .neqv..  Unless you intend to give .xor. a
> > different precedence, its implementation should be trivial (unless I
> > underestimate the difficulties a collision with a user-defined operator
> > could bring), so I'm wondering what kind of difficulties are you seeing?
>
> Exactly, the implementation should be almost trivial with ".xor." being
> treated as a synonym for ".neqv.".  The major difficulty I envisaged was
> in the parser.  Depending upon the applicable standard, ".xor." should
> either be recognized as an intrinsic silently, be matched as an intrinsic
> and issue a diagnostic warning or not be recognized and fall through to
> being treated as a general identifier (without type checking etc...)
>
> Unfortunately, the current structure of "gfc_match_strings" in addition to
> being inefficient, doesn't easily allow conditional keyword matching. 
> With the proposed scheme, supporting ".xor." might be as simple as
> something like:
>
>   case 'x':
>     if (gfc_next_char () == 'o'
>         && gfc_next_char () == 'r'
>         && gfc_next_char () == '.')
>       {
>         /* Matched ".xor.".  */
>         if (gfc_notify_std (GFC_STD_GNU, "...") == SUCCESS)
>           {
>             *result = INTRINSIC_NEQV;
>             return MATCH_YES;
>           }
>       }
>     break;
>
> Or alternatively, we could add a new INTRINSIC_XOR, to allow error
> messages and modules to display ".xor." instead or ".neqv.", and treat
> it as a synonym for INTRINSIC_NEQV everywhere.

In the current scheme, the following should be functionally equivalent 
to your suggestion:
Index: gcc/fortran/match.c
=================================================================
--- gcc/fortran/match.c (revision 122819)
+++ gcc/fortran/match.c (working copy)
@@ -44,6 +44,7 @@ mstring intrinsic_operators[] = {
     minit (".or.", INTRINSIC_OR),
     minit (".eqv.", INTRINSIC_EQV),
     minit (".neqv.", INTRINSIC_NEQV),
+    minit (".xor.", INTRINSIC_XOR), /* Need to declare I_XOR in 
gfortran.h  */
     minit (".eq.", INTRINSIC_EQ),
     minit ("==", INTRINSIC_EQ),
     minit (".ne.", INTRINSIC_NE),
@@ -485,6 +486,18 @@ gfc_match_intrinsic_op (gfc_intrinsic_op
   if (op == INTRINSIC_NONE)
     return MATCH_NO;

+  if (op == INTRINSIC_XOR)
+    {
+      if (gfc_notify_std (...) == FAILURE)
+       {
+         /* Will probably have to backup parser.  Fill in as
+            appropriate.   */
+         /* ... */
+         return MATCH_NO;
+       }
+      op = INTRINSIC_NEQV;
+    }
+
   *result = op;
   return MATCH_YES;
 }

I've not thought about interactions with user-defined .xor.s, but your 
patch would have the same problems.

I'd like to mention that I chose to return INTRINSIC_NEQV instead of 
INTRINSIC_XOR, because of the problem I had with my patch for PR 17711, 
where the introduction of new INTRINSIC_* values lead to issues with 
defined operators from modules.

> Whilst in theory, it is possible to add a wrapper around the current
> gfc_match_strings, remembering the state before, checking whether it
> returns INTRINSIC_XOR, and then deciding to either restrore
> gfc_current_locus and return MATCH_NO, or accept the ".xor." and transform
> it in INTRINSIC_NEQV, this overhead would make the current implementation
> even less efficient, and inserts more code to the common/critical path.
> As confirmed by FX, gfc_match_strings is already the hotest function in
> the gfortran front-end.

Presumably the biggest speedup can be obtained by eating ',' followed by 
whitespace before the call to gfc_match_strings in decl.c:2161, and 
matching double colons separately, as this is a case where valid code 
will have to backup insanely often.

If gfc_match_strings is still a hot function after this, I'm all for 
developing faster matching strategies.  But even then, I'm not thrilled 
by the loss in readability a patch like yours causes.

- Tobi