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

[Eric Christopher via cfe-commits]

On Sat, Mar 5, 2016 at 6:16 AM Daniel Sanders <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 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.
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.
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.

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160309/a98698f9/attachment-0001.html>