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 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. > > See below. > > 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. > > > 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. -eric > > > 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