<div dir="ltr">Hi Daniel,<div><br></div><div>Sorry for the delay, but I've been both away and catching up:<br><br><div class="gmail_quote"><div dir="ltr">On Wed, Mar 9, 2016 at 4:00 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">> > From: Eric Christopher [<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>]<br>
> > Sent: 09 March 2016 06:50<br>
> > To: <a href="mailto:reviews%2BD16139%2Bpublic%2B275805419034ae04@reviews.llvm.org" target="_blank">reviews+D16139+public+275805419034ae04@reviews.llvm.org</a>; Bhushan Attarde; Vasileios Kalintiris; Daniel Sanders<br>
> > Cc: Sagar Thakur; Nitesh Jain; Mohit Bhakkad; Jaydeep Patil; <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
> > Subject: Re: [PATCH] D16139: [MIPS] initFeatureMap() to handle empty string argument<br>
> ><br>
> > On Sat, Mar 5, 2016 at 6:16 AM Daniel Sanders <<a href="mailto:daniel.sanders@imgtec.com" target="_blank">daniel.sanders@imgtec.com</a><mailto:<a href="mailto:daniel.sanders@imgtec.com" target="_blank">daniel.sanders@imgtec.com</a>>> wrote:<br>
> > 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>
> > We already set a default CPU in the constructor (e.g. Mips32TargetInfoBase::Mips32TargetInfoBase() provides "mips32r2").<br>
> > It's the CPU argument to initFeatureMap() that's the root problem. In several targets, this argument has the same name as<br>
> > a member variable and is not subject to anything the constructor or setCPU() does to that member variable.<br>
><br>
> To be clear, no, this is not the problem.<br>
<br>
I can agree that there are additional problems (and that fixing them also fixes this problem) but I disagree that it's not a part of<br>
the problem. At the moment, I think we're both looking at different aspects of it and saying "this is the whole problem" and I<br>
think we've each missed the piece the other is looking at.<br>
<br>
Suppose TargetOptions::CPU is the empty string and TargetInfo::CreateTargetInfo() is called. The call to AllocateTarget() will leave<br>
MipsTargetInfoBase::CPU set to the default mips32r2 or mips64r2 (depending on the subclass). The call to MipsTargetInfoBase::setCPU()<br>
will not happen because the CPU is the empty string. Then when MipsTargetInfoBase::initFeatureMap() is called we have the following<br>
state:<br>
* MipsTargetInfoBase::CPU is mips32r2 or mips64r2<br>
* The CPU argument of initFeatureMap() is the empty string.<br>
The CPU name came from a single place but only one path resolved the empty string to a CPU name. I think this is wrong and that<br>
both paths should resolve to the default CPU, or preferably, there should only be one CPU variable.<br>
<br>
Let's consider something other than MIPS for a moment. I'll pick SystemZ because it's the only other target that initializes its CPU<br>
to a non-empty value in the constructor. In SystemZ, we have the following state for the above example:<br>
* SystemZTargetInfo::CPU is z10<br>
* The CPU argument of initFeatureMap() is the empty string.<br>
Now, SystemZTargetInfo::initFeatureMap() doesn't have any checks for CPU == "z10" but if it did there would be a difference in<br>
behaviour between the default 'z10' and an explicit 'z10' since CPU == "z10" would be false in the default 'z10' case (because CPU<br>
would be the empty string).<br>
<br>
Going back to MIPS, MipsTargetInfoBase::initFeatureMap() does encounter a difference between a default 'mips32r2' and an explicit<br>
'mips32r2' because of the 'Features[CPU] = true' line. The clang driver currently makes sure we're always explicit but lldb doesn't have this.<br>
<br>
Fixing the above inconsistency would resolve the problem by itself, but I do agree that we're also handling the CPU name incorrectly<br>
in MipsTargetInfoBase::initFeatureMap(). I agree that the 'Features[CPU] = true' is bad and fixing that should also resolve the problem by<br>
itself. However, it would leave this weird inconsistency between the default 'mips32r2' and the explicit 'mips32r2'.<br>
<br>
I'm also wondering if the 'Features[CPU] = true' line might be redundant since the backend Processor<> and ProcessorModel<><br>
definitions should have the same effect. I'll have to look into that when I get chance.<br>
<br></blockquote><div><br></div><div>It should. Anything else should be a bug.</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>
> This is also not the problem. There are a few problems here:<br>
><br>
> 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.<br>
> a) There should be a testcase, everything can be done by the driver here as the code is pretty specialized for that use case.<br>
<br>
The test case is intended to be the lldb testsuite, without it lldb emits countless warnings about the '+' feature. I'm not aware of a<br>
way to trigger the problem from the clang driver since it always passes an explicit CPU name. As a result, I'm don't know of a way to<br>
test on the clang side.<br>
<br></blockquote><div><br></div><div>See below.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> 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.<br>
<br>
I agree, we have two separate concepts that happen to use the same strings. We should probably map them explicitly.<br></blockquote><div><br></div><div>Yes, and that's what you should do instead of this patch.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> 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:<br>
><br>
> case llvm::Triple::mips:<br>
> case llvm::Triple::mipsel:<br>
> case llvm::Triple::mips64:<br>
> case llvm::Triple::mips64el: {<br>
> StringRef CPUName;<br>
> StringRef ABIName;<br>
> mips::getMipsCPUAndABI(Args, T, CPUName, ABIName);<br>
> return CPUName;<br>
> }<br>
><br>
> for mips.<br>
><br>
> 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.<br>
<br>
This usage is from the clang driver. On this path, getMipsCPUAndABI ensures that the CPU is never empty.<br></blockquote><div><br></div><div>I gave you a testcase that can prove otherwise in my earlier email.</div><div><br></div><div>-eric</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> 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 :)<br>
><br>
> -eric</blockquote></div></div></div>