<div dir="ltr">Thanks. I'll split the patch into two parts (the NFC refactoring part and the rest) and commit them.<br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, May 6, 2015 at 2:54 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Cool deal. Feel free to commit after.<br><br>Thanks for the work here.<span class="HOEnZb"><font color="#888888"><div><br></div><div>-eric</div></font></span></div><div class="HOEnZb"><div class="h5"><br><div class="gmail_quote">On Wed, May 6, 2015 at 2:53 PM Akira Hatanaka <<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, May 6, 2015 at 1:55 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><span>On Wed, May 6, 2015 at 1:51 PM Akira Hatanaka <<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">In <a href="http://reviews.llvm.org/D9537#167157" target="_blank">http://reviews.llvm.org/D9537#167157</a>, @echristo wrote:<br>
<br>
> I agree, no TargetOptions for this. I don't necessarily agree with rewriting, but it seems to be a small enough change and will work for what the overrides are meant to be for. Some discipline in reviewing testcases to make sure that a lot of overrides aren't there for incoming testcases is going to be necessary for clean tests. One inline comment which basically sums up to "why not two functions"? As long as you have a reasonable answer go ahead.<br>
<br>
<br>
I'm not sure if I understood your comment about testcases. Do you mean people shouldn't duplicate functions in test cases just to test different target-cpu and target-feature strings but instead should use -mcpu and -mattrs (which I think makes sense)? Should the one inline comment go into the test cases I added?<br>
<br>
<br></blockquote><div><br></div></span><div>My comment about testcases is that someone shouldn't have:</div><div><br></div><div>; RUN: llc -march=foo</div><div><br></div><div>as the only run line with a function with:</div><div><br></div><div>target-cpu="notfoo"</div><div><br></div><div>when they could just change the arch on the function to say "foo" rather than "notfoo".</div><span><div> </div></span></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>OK, I agree. In that case, people should be advised to remove the command line option and change the "target-cpu" attribute or just remove the "target-cpu" attribute.</div><div> </div><div>I'll add comments to the test cases in this patch to make it clear what they are testing.</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
================<br>
Comment at: include/llvm/CodeGen/CommandFlags.h:268<br>
@@ -262,1 +267,3 @@<br>
<br>
+static inline std::pair<std::string, std::string> getCPUFeaturesStr() {<br>
+  std::string CPUStr;<br>
----------------<br>
echristo wrote:<br>
> Why a pair?<br>
Is it better to split this into two functions getCPUStr and getFeaturesStr? Or you meant the two strings (cpu and features strings) should be passed by reference?<br>
<br></blockquote><div><br></div></span><div>I meant split into two functions, any particular reason?</div><span><font color="#888888"><div><br></div></font></span></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>No particular reason. I'll split the function into two.</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><span><font color="#888888"><div></div><div>-eric</div></font></span><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<a href="http://reviews.llvm.org/D9537" target="_blank">http://reviews.llvm.org/D9537</a><br>
<br>
EMAIL PREFERENCES<br>
  <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br>
</blockquote></span></div></div>
</blockquote></div></div></div></blockquote></div>
</div></div></blockquote></div><br></div></div>