[PATCH] [tablegen] Resolve complex def identifiers when instanciating multiclass

Hal Finkel hfinkel at anl.gov
Tue Jul 8 07:07:32 PDT 2014 

----- Original Message -----
> From: "deadal nix" <deadalnix at gmail.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "llvm-commits" <llvm-commits at cs.uiuc.edu>
> Sent: Friday, July 4, 2014 10:06:48 PM
> Subject: Re: [PATCH] [tablegen] Resolve complex def identifiers when instanciating multiclass
> 
> 
> Updated patch with explanations in comments.
> 

Okay, please also add a comment directly above the call to ResolveMulticlassDefArgs explaining that it is being called here, earlier than usual, so that the complex pattern name can be resolved without turning the whole thing into a string that would have a prefix appended. Also, please add a comment to ResolveMulticlassDefArgs noting that it can be called twice (and specify the circumstances under which that might happen). With this additional commentary, LGTM. Thanks!

 -Hal

> 
> 
> 
> 2014-07-02 2:37 GMT-07:00 Hal Finkel < hfinkel at anl.gov > :
> 
> 
> 
> ----- Original Message -----
> > From: "deadal nix" < deadalnix at gmail.com >
> > To: "Hal Finkel" < hfinkel at anl.gov >
> > Cc: "llvm-commits" < llvm-commits at cs.uiuc.edu >
> 
> > Sent: Wednesday, July 2, 2014 1:50:31 AM
> > Subject: Re: [PATCH] [tablegen] Resolve complex def identifiers
> > when instanciating multiclass
> > 
> 
> > No that wouldn't work, and I tried to explain it, but apparently I
> > wasn't very clear. Sorry for that.
> > 
> > The name is of the def is computed the following way:
> > - If it is a simple string, then prepend the prefix and use that.
> > - If it is something more complex, then resolve that thing and use
> > that.
> > 
> > If you eagerly resolve arguments, then everything become a string,
> > and the prefix is added every time. I do think that this would have
> > been a sensible thing to do when initially working on tablegen, but
> > that would break most backend, so i don't think that is is
> > something
> > we want to do now.
> > 
> > Is that clearer ?
> > 
> 
> Yes, please add to the patch a detailed comment to the code
> explaining this.
> 
> -Hal
> 
> 
> 
> > 
> > 2014-06-30 21:15 GMT-07:00 Hal Finkel < hfinkel at anl.gov > :
> > 
> > 
> > 
> > ----- Original Message -----
> > > From: "deadal nix" < deadalnix at gmail.com >
> > 
> > > To: "Hal Finkel" < hfinkel at anl.gov >
> > > Cc: "llvm-commits" < llvm-commits at cs.uiuc.edu >
> > > Sent: Monday, June 30, 2014 10:49:28 PM
> > > Subject: Re: [PATCH] [tablegen] Resolve complex def identifiers
> > > when instanciating multiclass
> > > 
> > > 
> > 
> > > As per comment, I removed the {}.
> > > 
> > > 
> > > 
> > > 
> > > 2014-06-30 20:24 GMT-07:00 deadal nix < deadalnix at gmail.com > :
> > > 
> > > 
> > > 
> > > 
> > > The resolution is done after the name resolution because the name
> > > creation depend on it. If the name is a simple string, then the
> > > multiclass prefix is not added. That seems like a bizarre design
> > > to
> > > me but it is already workign that way, so I just extended the
> > > existing design.
> > 
> > I don't understand what you mean. To be more specific, what if you:
> > 
> > 1. Removed the call to ResolveMulticlassDefArgs in
> > TGParser::ParseDefm
> > 
> > 2. Changed your patch so that the logic read:
> > 
> > 
> > + ResolveMulticlassDefArgs(MC, CurRec, DefmPrefixRange.Start,
> > Lex.getLoc(),
> > + TArgs, TemplateVals, false/*Delete args*/);
> > +
> > + DefName = CurRec->getNameInit();
> > + DefNameString = dyn_cast<StringInit>(DefName);
> > 
> > +
> > + if (!DefNameString)
> > + DefName = DefName->convertInitializerTo(StringRecTy::get());
> > ...
> > 
> > so that we always call ResolveMulticlassDefArgs and then try to
> > resolve the name in InstantiateMulticlassDef. Is there some reason
> > that would not work? It seems as though it would be cleaner.
> > 
> > -Hal
> > 
> > 
> > 
> > > 
> > > Changing this design would probably break most backends, and it
> > > is
> > > probably not worth it. Removing the {} and coming back with an
> > > updated patch.
> > > 
> > > 
> > > 
> > > 
> > > 2014-06-30 11:39 GMT-07:00 Hal Finkel < hfinkel at anl.gov > :
> > > 
> > > 
> > > 
> > > 
> > > ----- Original Message -----
> > > > From: "deadal nix" < deadalnix at gmail.com >
> > > > To: llvm-commits at cs.uiuc.edu
> > > > Sent: Monday, June 30, 2014 1:24:44 PM
> > > > Subject: Re: [PATCH] [tablegen] Resolve complex def identifiers
> > > > when instanciating multiclass
> > > > 
> > > > 
> > > > 
> > > > PING ?
> > > > 
> > > 
> > > Please wait 1 week in between pings for non-urgent patches. That
> > > having been said, thanks for working on this.
> > > 
> > > + DefName = CurRec->getNameInit();
> > > + DefNameString = dyn_cast<StringInit>(DefName);
> > > +
> > > + // OK the pattern is more complex than simply using NAME.
> > > + // Let's use the heavy weaponery.
> > > + if (!DefNameString) {
> > > + ResolveMulticlassDefArgs(MC, CurRec, DefmPrefixRange.Start,
> > > Lex.getLoc(),
> > > + TArgs, TemplateVals, false/*Delete args*/);
> > > +
> > > + DefName = CurRec->getNameInit();
> > > + DefNameString = dyn_cast<StringInit>(DefName);
> > > 
> > > Why are we only calling ResolveMulticlassDefArgs when we can't
> > > resolve the name? It seems somewhat funny to have the argument
> > > resolution state depend on the kind of name that is used. Does
> > > something break if you always call this?
> > > 
> > > 
> > > + if (!DefNameString) {
> > > + DefName = DefName->convertInitializerTo(StringRecTy::get());
> > > + }
> > > 
> > > Remove the {} here.
> > > 
> > > Thanks,
> > > Hal
> > > 
> > > 
> > > 
> > > > 
> > > > 
> > > > 
> > > > 2014-06-26 23:27 GMT-07:00 deadal nix < deadalnix at gmail.com > :
> > > > 
> > > > 
> > > > 
> > > > ping !!?!
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 2014-06-24 16:02 GMT-07:00 deadal nix < deadalnix at gmail.com > :
> > > > 
> > > > 
> > > > 
> > > > 
> > > > ping \o/
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 2014-06-23 0:22 GMT-07:00 deadal nix < deadalnix at gmail.com > :
> > > > 
> > > > 
> > > > 
> > > > 
> > > > As per title. I ran into tablegen limitations that are
> > > > annoying,
> > > > namely that I couldn't generate custom name for def inside
> > > > multiclass. So here is the fix.
> > > > 
> > > > 
> > > > 
> > > > 
> > > > _______________________________________________
> > > > llvm-commits mailing list
> > > > llvm-commits at cs.uiuc.edu
> > > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> > > > 
> > > 
> > > --
> > > Hal Finkel
> > > Assistant Computational Scientist
> > > Leadership Computing Facility
> > > Argonne National Laboratory
> > > 
> > > 
> > > 
> > 
> > --
> > Hal Finkel
> > Assistant Computational Scientist
> > Leadership Computing Facility
> > Argonne National Laboratory
> > 
> > 
> 
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory

More information about the llvm-commits mailing list