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

[Hal Finkel]

----- 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