<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Sat, Mar 5, 2016 at 6:16 AM Daniel Sanders <<a href="mailto:daniel.sanders@imgtec.com">daniel.sanders@imgtec.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">dsanders added a comment.<br>
<br>
In <a href="http://reviews.llvm.org/D16139#368217" rel="noreferrer" target="_blank">http://reviews.llvm.org/D16139#368217</a>, @echristo wrote:<br>
<br>
> This seems wrong. You should fix setCPU instead or set a default CPU.<br>
<br>
<br>
We already set a default CPU in the constructor (e.g. Mips32TargetInfoBase::Mips32TargetInfoBase() provides "mips32r2"). It's the CPU argument to initFeatureMap() that's the root problem. In several targets, this argument has the same name as a member variable and is not subject to anything the constructor or setCPU() does to that member variable.<br>
<br></blockquote><div><br></div><div>To be clear, no, this is not the problem.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I suspect the right thing to do is to drop the CPU argument and use the member variable instead but there may be differences in value/usage that make this difficult. For now, this patch serves as a stop-gap measure that resolves the empty string to a real CPU name.<br><br></blockquote><div><br></div><div>This is also not the problem. There are a few problems here:</div><div><br></div><div>z) This code is terrible, I did my best to clean it up recently, but it's a lot of code and a bit painful.</div><div>a) There should be a testcase, everything can be done by the driver here as the code is pretty specialized for that use case.</div><div>b) CPUs are not subtarget features (or they shouldn't be), they're CPUs that contain features. They may be generic names for ISAs as well, but probably best to keep them separate.</div><div>c) You should set the features based on the CPUs given to the function. The typical way the cpu comes in, is via -target-cpu which comes via:</div><div><br></div><div><div>  case llvm::Triple::mips:</div><div>  case llvm::Triple::mipsel:</div><div>  case llvm::Triple::mips64:</div><div>  case llvm::Triple::mips64el: {</div><div>    StringRef CPUName;</div><div>    StringRef ABIName;</div><div>    mips::getMipsCPUAndABI(Args, T, CPUName, ABIName);</div><div>    return CPUName;</div><div>  }</div></div><div><br></div><div>for mips.</div><div><br></div><div>Now if your triple is returning an empty string here you might have gotten to where you are (I tried mips64r2-linux-gnu as the -target option). Which is what typically happens down this path. </div><div><br></div><div>As I said, I agree the code along this path is terrible, but I don't think this change is correct - and it should have had a testcase anyhow :)</div><div><br></div><div>-eric</div></div></div>