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

[Daniel Sanders via cfe-commits]

dsanders added a comment.

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


Repository:
  rL LLVM

http://reviews.llvm.org/D16139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eric Christopher via cfe-commits]

Hi Daniel,

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

On Wed, Mar 9, 2016 at 4:00 AM Daniel Sanders <daniel.sand...@imgtec.com>
wrote:

> > > From: Eric Christopher [echri...@gmail.com]
> > > Sent: 09 March 2016 06:50
> > > To: reviews+d16139+public+275805419034a...@reviews.llvm.org; Bhushan
> Attarde; Vasileios Kalintiris; Daniel Sanders
> > > Cc: Sagar Thakur; Nitesh Jain; Mohit Bhakkad; Jaydeep Patil;
> cfe-commits@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.sand...@imgtec.com<mailto:daniel.sand...@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.
>
>
It should. Anything else should be a bug.


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

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

[Daniel Sanders via cfe-commits]

> > From: Eric Christopher [echri...@gmail.com]
> > Sent: 09 March 2016 06:50
> > To: reviews+d16139+public+275805419034a...@reviews.llvm.org; Bhushan 
> > Attarde; Vasileios Kalintiris; Daniel Sanders
> > Cc: Sagar Thakur; Nitesh Jain; Mohit Bhakkad; Jaydeep Patil; 
> > cfe-commits@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.sand...@imgtec.com<mailto:daniel.sand...@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 ISA

Re: [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 
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Daniel Sanders via cfe-commits]

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.

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.


Repository:
  rL LLVM

http://reviews.llvm.org/D16139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eric Christopher via cfe-commits]

echristo added a subscriber: echristo.
echristo added a comment.

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


Repository:
  rL LLVM

http://reviews.llvm.org/D16139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Daniel Sanders via cfe-commits]

dsanders accepted this revision.
dsanders added a comment.

Thanks. LGTM


Repository:
  rL LLVM

http://reviews.llvm.org/D16139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Bhushan Attarde via cfe-commits]

bhushan added a comment.

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


Repository:
  rL LLVM

http://reviews.llvm.org/D16139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Daniel Sanders via cfe-commits]

dsanders added a comment.

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.


Repository:
  rL LLVM

http://reviews.llvm.org/D16139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Bhushan Attarde via cfe-commits]

bhushan added a comment.

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


Repository:
  rL LLVM

http://reviews.llvm.org/D16139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Vasileios Kalintiris via cfe-commits]

vkalintiris accepted this revision.
vkalintiris added a comment.
This revision is now accepted and ready to land.

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


Repository:
  rL LLVM

http://reviews.llvm.org/D16139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits