<div dir="ltr"><div style>Hi Bill,</div><div style><br></div><div>> +    TM.cloneWithOptions(mf);</div><div><br></div><div style>cloneWithOptions is declared to return a pointer. Since this is the only place where it's called, should it return void instead?</div>
<div style><br></div><div style>Also, the word "clone" has the connotation that a new object is being created, but what this actually appears to do is reconstruct an object in place.</div><div style><br></div><div style>
> +  this->~ARMTargetMachine();</div><div>> +  TargetMachine *NewTM = new (this)</div><div>> +    ARMTargetMachine(TheTarget, targetTriple, targetCPU, targetFS,</div><div>> +                     Opts, RM, CM, OptLvl);</div>
<div>> +</div><div>> +  if (TPC)</div><div>> +    PassConfig = TPC->cloneWithTargetMachine(NewTM);</div><div><br></div><div style>There is some disagreement about whether this code is valid (specifically, whether it's valid to reference members of this after the placement new). I think this code would be clearer and safer if TargetMachine and its subclasses merely had a plain method which could re-initialize all needed fields in place, instead of using placement new. Any reference members that they contain would need to be changed to pointers, so that they can be reassigned, but I think the code would be cleaner, and it'd have less risk of violating C++ rules.</div>
<div style><br></div><div style><div>> -  assert(AsmInfo && "MCAsmInfo not initialized."</div><div>> +  assert(AsmInfo && "MCAsmInfo not initialized. "</div><div><br></div><div>This probably isn't supposed to be in the patch.</div>
<div><br></div></div><div style>Dan</div><div style><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jun 5, 2013 at 1:05 AM, Bill Wendling <span dir="ltr"><<a href="mailto:wendling@apple.com" target="_blank">wendling@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Here's a better patch (I missed some code in one of the 'clones').<br>
<span class="HOEnZb"><font color="#888888"><br>
-bw<br>
<br>
</font></span><br><br>
<br>
On Jun 5, 2013, at 12:12 AM, Bill Wendling <<a href="mailto:wendling@apple.com">wendling@apple.com</a>> wrote:<br>
<br>
> Hi all!<br>
><br>
> This patch is the culmination of the attributes changes I did last year. It changes the back-end when target options change. For instance, if one function was compiled with the `-msoft-float' command line option and another was compiled without that flag, then we want functions to generate the correct code, even if their modules are merged.<br>

><br>
> So what does this patch do?<br>
><br>
> When the target options change, it regenerates the TargetMachine object. It does this using a placement-new method, because a reference to the TargetMachine object is stored in several places. So it's important to reuse the memory address. (The same is true for the TargetConfigPass object that the TargetMachine creates.)<br>

><br>
> Why do we need to regenerate the TargetMachine?<br>
><br>
> The back-end code was built around the assumption that once the TargetMachine object was generated, none of the state it built up would change. For instance, how instruction selection treats specific types may depend on the target options. It is a very long-term project to move to a system where the function's attributes are checked directly. That said, this isn't a "hack", but could be looked upon as an intermediate step from what we have to what would be ideal.<br>

><br>
> I ran some tests, and there doesn't appear to be a difference in compilation time for the normal case --- one where the module contains functions generated with the same command line options.<br>
><br>
> Please review this patch and submit your comments and flamage.<br>
><br>
> -bw<br>
><br>
> <changing-target-options.patch><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>
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></blockquote></div><br></div>