<div dir="ltr">Updated patch with explanations in comments.<br></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-07-02 2:37 GMT-07:00 Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span>:<br>

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