[PATCH] D16139: [MIPS] initFeatureMap() to handle empty string argument

[Daniel Sanders via cfe-commits]

> > From: Eric Christopher [echristo at gmail.com]
> > Sent: 09 March 2016 06:50
> > To: reviews+D16139+public+275805419034ae04 at reviews.llvm.org; Bhushan Attarde; Vasileios Kalintiris; Daniel Sanders
> > Cc: Sagar Thakur; Nitesh Jain; Mohit Bhakkad; Jaydeep Patil; cfe-commits at lists.llvm.org
> > Subject: Re: [PATCH] D16139: [MIPS] initFeatureMap() to handle empty string argument
> > 
> > On Sat, Mar 5, 2016 at 6:16 AM Daniel Sanders <daniel.sanders at imgtec.com<mailto:daniel.sanders at imgtec.com>> wrote:
> > dsanders added a comment.
> > 
> > In http://reviews.llvm.org/D16139#368217, @echristo wrote:
> > 
> > > This seems wrong. You should fix setCPU instead or set a default CPU.
> > 
> > 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.
> 
> To be clear, no, this is not the problem.

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
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
think we've each missed the piece the other is looking at.

Suppose TargetOptions::CPU is the empty string and TargetInfo::CreateTargetInfo() is called. The call to AllocateTarget() will leave
MipsTargetInfoBase::CPU set to the default mips32r2 or mips64r2 (depending on the subclass). The call to MipsTargetInfoBase::setCPU()
will not happen because the CPU is the empty string. Then when MipsTargetInfoBase::initFeatureMap() is called we have the following
state:
* MipsTargetInfoBase::CPU is mips32r2 or mips64r2
* The CPU argument of initFeatureMap() is the empty string.
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
both paths should resolve to the default CPU, or preferably, there should only be one CPU variable.

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
to a non-empty value in the constructor. In SystemZ, we have the following state for the above example:
* SystemZTargetInfo::CPU is z10
* The CPU argument of initFeatureMap() is the empty string.
Now, SystemZTargetInfo::initFeatureMap() doesn't have any checks for CPU == "z10" but if it did there would be a difference in
behaviour between the default 'z10' and an explicit 'z10' since CPU == "z10" would be false in the default 'z10' case (because CPU
would be the empty string).

Going back to MIPS, MipsTargetInfoBase::initFeatureMap() does encounter a difference between a default 'mips32r2' and an explicit
'mips32r2' because of the 'Features[CPU] = true' line. The clang driver currently makes sure we're always explicit but lldb doesn't have this.

Fixing the above inconsistency would resolve the problem by itself, but I do agree that we're also handling the CPU name incorrectly
in MipsTargetInfoBase::initFeatureMap(). I agree that the 'Features[CPU] = true' is bad and fixing that should also resolve the problem by
itself. However, it would leave this weird inconsistency between the default 'mips32r2' and the explicit 'mips32r2'.

I'm also wondering if the 'Features[CPU] = true' line might be redundant since the backend Processor<> and ProcessorModel<>
definitions should have the same effect. I'll have to look into that when I get chance.

> > 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.
> 
> This is also not the problem. There are a few problems here:
> 
> 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.
> a) There should be a testcase, everything can be done by the driver here as the code is pretty specialized for that use case.

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
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
test on the clang side.

> 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.

I agree, we have two separate concepts that happen to use the same strings. We should probably map them explicitly.

> 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:
> 
>   case llvm::Triple::mips:
>   case llvm::Triple::mipsel:
>   case llvm::Triple::mips64:
>   case llvm::Triple::mips64el: {
>     StringRef CPUName;
>     StringRef ABIName;
>     mips::getMipsCPUAndABI(Args, T, CPUName, ABIName);
>     return CPUName;
>   }
> 
> for mips.
> 
> 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.

This usage is from the clang driver. On this path, getMipsCPUAndABI ensures that the CPU is never empty.

> 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 :)
> 
> -eric