[MIPS] initFeatureMap() to handle empty string argument

[bhushan]

Summary

This patch sets CPU string to its default value when it is not supplied by caller.

[vkalintiris]

LGTM. However, I'm curious to learn which case triggered this behaviour.

[bhushan]

This was observed during expression evaluation in LLDB, where LLDB does not explicitly set clang::TargetOptions::CPU hence it remains empty.

[dsanders]

Can you explain what the problem was and why this change is needed?

I'm guessing it's something to do with the 'Features[CPU] = true' line.

[bhushan]

I'm guessing it's something to do with the 'Features[CPU] = true' line.

Yes.
Features[CPU] = true enables a feature given by CPU string. (This gets translated into "+CPU").
When CPU string is empty, this gets translated into "+" (i.e. with empty feature name).

This results into a warning '+' is not a recognized feature for this target (ignoring feature).

[dsanders]

Thanks. LGTM

[echristo]

This seems wrong. You should fix setCPU instead or set a default CPU.

[dsanders]

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

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.

[dsanders]

From: Eric Christopher [[e-mail]]
Sent: 09 March 2016 06:50
To: [e-mail]; Bhushan Attarde; Vasileios Kalintiris; Daniel Sanders
Cc: Sagar Thakur; Nitesh Jain; Mohit Bhakkad; Jaydeep Patil; [e-mail]
Subject: Re: [PATCH] D16139: [MIPS] initFeatureMap() to handle empty string argument

On Sat, Mar 5, 2016 at 6:16 AM Daniel Sanders <[e-mail]<mailto:[e-mail]>> 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

[echristo]

Hi Daniel,

Sorry for the delay, but I've been both away and catching up:

[dsanders]

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.

Yes, and that's what you should do instead of this patch.

Just making the mapping explicit isn't enough by itself. We still have to resolve the empty string to the default CPU because initFeatureMap()'s CPU argument dodges the defaults that are normally handled in the constructor. I'll post patches soon (I've been away for a few days too) I just need to test a couple changes we're agreed on first and sort out some remaining IAS work.

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.

I gave you a testcase that can prove otherwise in my earlier email.

I don't think it proves otherwise. The triple you quoted isn't a supported MIPS triple and LLVM will reject it. LLVM for MIPS doesn't accept CPU names in the first component of the triple and this particular one isn't known to gcc either. Can you give me the exact command you tried? I get this:

$ bin/clang -target mips64r2-linux-gnu -o hello.s -S hello.c
error: unknown target triple 'mips64r2--linux-gnu', please use -triple or -arch
$ bin/llc -mtriple mips64r2-linux-gnu -o hello.s hello.bc
bin/llc: : error: unable to get target for 'mips64r2--linux-gnu', see --version and --triple.

and if a triple that wasn't mips/mipsel/mips64/mips64el somehow got in to getMipsCPUAndABI() it would trigger an llvm_unreachable().

It's impossible to provide a known MIPS triple and end up with the empty string as the CPU name within the clang driver. The empty string is only known to occur from LLDB's usage.

lib/Basic/Targets.cpp

return llvm::StringSwitch<bool>(Name)
         .Case("p5600", true)
         .Default(false);
   }
   const std::string& getCPU() const { return CPU; }
   bool
   initFeatureMap(llvm::StringMap<bool> &Features, DiagnosticsEngine &Diags,
                  StringRef CPU,
                  const std::vector<std::string> &FeaturesVec) const override {
+    if (CPU.empty())
+        CPU = getCPU();
     if (CPU == "octeon")
       Features["mips64r2"] = Features["cnmips"] = true;
     else
       Features[CPU] = true;
     return TargetInfo::initFeatureMap(Features, Diags, CPU, FeaturesVec);
   }
 
   void getTargetDefines(const LangOptions &Opts,