Oops - copying cfe-commits in. (Still LGTM)
> -----Original Message----- > From: Bernard Ogden > Sent: 22 January 2014 12:30 > To: [email protected] > Cc: [email protected] > Subject: RE: [PATCH] add predefined macros for thumbv8 > > LGTM. > > Thanks for all the tweaks. > > Regards, > > Bernie > > > -----Original Message----- > > From: Weiming Zhao [mailto:[email protected]] > > Sent: 21 January 2014 18:55 > > To: Bernard Ogden > > Cc: [email protected] > > Subject: RE: [PATCH] add predefined macros for thumbv8 > > > > Hi Bernie, > > > > Thanks for clarification. Now it makes more sense. :D > > Attached is the patch. I also removed some spaces in two empty lines. > > > > Thanks, > > Weiming > > > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > > hosted by > > The Linux Foundation > > > > > > -----Original Message----- > > From: Bernie Ogden [mailto:[email protected]] > > Sent: Tuesday, January 21, 2014 12:44 AM > > To: [email protected] > > Cc: [email protected] > > Subject: RE: [PATCH] add predefined macros for thumbv8 > > > > Hi Weiming, > > > > I agree with you about how the driver should behave. > > > > I think the patch is nearly there, but I meant to add the thumbv* > > checks for > > __thumb2__ rather than interwork. Doing these checks for interwork is > > harmless but unnecessary, as 1136 will set this predefine. But you > _do_ > > need > > them to get thumb2 defined for -triple thumbv6t2 and -triple thumbv7. > > You > > could get away with just using ArchName.endswith for this, as the > > relevant > > code is already guarded by if(IsThumb). > > > > It would be worth adding tests for those two cases as well, and for a > > non-thumb2 case, i.e. something like: > > > > %clang_cc1 -E -dM -ffreestanding -triple=thumbv5 < /dev/null | > > FileCheck > > -check-prefix Thumbv5 > > // Thumbv5: #define __THUMB_INTERWORK__ 1 > > // Thumbv5-NOT: #define __thumb2__ > > %clang_cc1 -E -dM -ffreestanding -triple=thumbv6t2 < /dev/null | > > FileCheck > > -check-prefix Thumbv6t2 > > // Thumbv6t2: #define __THUMB_INTERWORK__ 1 > > // Thumbv6t2: #define __thumb2__ > > %clang_cc1 -E -dM -ffreestanding -triple=thumbv7 < /dev/null | > > FileCheck > > -check-prefix Thumbv7 > > // Thumbv7: #define __THUMB_INTERWORK__ 1 > > // Thumbv7: #define __thumb2__ > > > > Makes sense? > > > > Regards, > > > > Bernie > > > > > > > -----Original Message----- > > > From: Weiming Zhao [mailto:[email protected]] > > > Sent: 21 January 2014 01:34 > > > To: Bernard Ogden > > > Cc: [email protected] > > > Subject: RE: [PATCH] add predefined macros for thumbv8 > > > > > > Hi Bernie, > > > > > > Thanks for the comments. > > > I agree we should first fix the correctness issue (predefined > macros) > > > for > > > thumbv8. > > > Attached is the change as you suggested. > > > > > > PS. I see Driver/Tools.cpp has getARMCPUForMArch() that infers cpu > > from > > > arch. > > > So I think, if either mcpu or triple is specified, the driver > should > > > figure > > > out the other one instead of using the default. > > > When the two flags conflict, it should give diagnostics. Currently, > > if > > > I > > > give "clang -mcpu=cortex-a7 -target armv8 ", it doesn't complain. > > > > > > Thanks, > > > Weiming > > > > > > > > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > > > hosted by > > > The Linux Foundation > > > > > > -----Original Message----- > > > From: Bernie Ogden [mailto:[email protected]] > > > Sent: Monday, January 20, 2014 4:32 AM > > > To: [email protected] > > > Cc: [email protected] > > > Subject: RE: [PATCH] add predefined macros for thumbv8 > > > > > > Ah, ARMTargetInfo's constructor sets the CPU to 1136. Even if the > > arch > > > in > > > the triple is < v6. Hmm. > > > > > > This seems to trace back to the introduction of setCPU in > > > https://llvm.org/svn/llvm-project/cfe/trunk@91700 - which at the > time > > > had a > > > FIXME attached to it, but the FIXME is now gone and setCPU seems to > > be > > > used > > > across many architectures. > > > > > > I still think that's a bug, but someone with more knowledge than me > > may > > > think differently. I think it could be fixed a few ways: > > > > > > 1) Stop deriving the arch from the CPU. We've got the triple, > derive > > > the > > > arch from that. But that overrides the general rule that the target > > > arch > > > just sets a default CPU and could open up a can of worms - what if > I > > > set > > > -target arm, or -target armv8 and then a pre-v8 CPU? > > > 2) Set predefines based on subtarget features. This can't cover all > > > cases, > > > though - e.g. interworking. > > > 3) Pull the mapping in getARMCPUForMArch out to somewhere it's > > > accessible > > > from cc1, as well as the driver. Then make the ARMTargetInfo > > > constructor set > > > a default CPU based on the arch, rather than hard-coding 1136. > > > > > > All of which could take a while to sort out... so I think you > should > > go > > > ahead and commit your patch, but modify it to set __thumb2__ for > > > triples > > > ending in v6t2 and v7 as well, and perhaps add a comment explaining > > why > > > this > > > is necessary. > > > > > > And thanks for looking into the problem :) > > > > > > Regards, > > > > > > Bernie > > > > > > > -----Original Message----- > > > > From: [email protected] [mailto:[email protected]] > > > > Sent: 20 January 2014 01:03 > > > > To: Bernard Ogden > > > > Cc: [email protected]; [email protected] > > > > Subject: RE: [PATCH] add predefined macros for thumbv8 > > > > > > > > Hi Bernie, > > > > > > > > It doesn't seem to be a bug. > > > > when passing "-cc1 -E -dM -ffreestanding -triple=thumbv8" to > clang, > > > > the default cpu is arm1136j-s, so CPUArchVer is 6. Sometime, the > > > > triple and mcpu flag may be insconsistent. > > > > > > > > > > > > Thanks, > > > > Weiming > > > > > > > > > > > > > (Failed to send to list - apologies to Weiming for the double > > > > > reply.) > > > > > > > > > > > > > > > > > > > > That sounds like a further bug - thumbv8 really shouldn't > result > > > in > > > > the > > > > > arch ver appearing to be 6. I think the right way to deal with > > this > > > > is to > > > > > fix the underlying bug. Would you mind looking into it? > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > > > > Bernie > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > From: Weiming Zhao [mailto:[email protected]] > > > > > Sent: 15 January 2014 22:49 > > > > > To: Bernard Ogden; [email protected] > > > > > Subject: RE: [PATCH] add predefined macros for thumbv8 > > > > > > > > > > > > > > > > > > > > Hi Bernie, > > > > > > > > > > > > > > > > > > > > Thanks for reviewing. I changed that check to "CPUArchVer >=7" > > > > > > > > > > > > > > > > > > > > The reason for checking of ArchName is if I just pass > > > triple=thumbv8 > > > > (e.g. > > > > > -cc1 -E -dM -ffreestanding -triple=thumbv8), the CPUArchVer is > > > still > > > > 6 > > > > > unless mcpu is set to cortex-a53. > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > Weiming > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Qualcomm Innovation Center, Inc. is a member of Code Aurora > > Forum, > > > > hosted > > > > > by > > > > > The Linux Foundation > > > > > > > > > > > > > > > > > > > > From: Bernie Ogden [mailto:[email protected]] > > > > > Sent: Wednesday, January 15, 2014 4:22 AM > > > > > To: [email protected]; [email protected] > > > > > Subject: RE: [PATCH] add predefined macros for thumbv8 > > > > > > > > > > > > > > > > > > > > Hi Weiming, > > > > > > > > > > > > > > > > > > > > I think __thumb2_ should be set regardless of whether we're > > > > generating ARM > > > > > or Thumb code - and I think this is what your patch does. Isn't > > the > > > > check > > > > > of > > > > > the ArchName string redundant with the check for CPUArchVer == > 8? > > > If > > > > I'm > > > > > right then the tests need a corresponding adjustment. > > > > > > > > > > > > > > > > > > > > I'd also slightly prefer that you check for CPUArchVer >= 7, > > rather > > > > than > > > > > checking for == 7 and ==8, but I don't feel all that strongly > > about > > > > it. > > > > > > > > > > > > > > > > > > > > Looks like we should consider moving the tests in > > > > > Preprocessor/arm-target-features.c into Preprocessor/init.c, > but > > > > that's > > > > > not > > > > > your problem :) > > > > > > > > > > > > > > > > > > > > Regards, > > > > > > > > > > > > > > > > > > > > Bernie > > > > > > > > > > > > > > > > > > > > From: [email protected] > > > > > [mailto:[email protected]] On Behalf Of Weiming > > Zhao > > > > > Sent: 14 January 2014 01:03 > > > > > To: [email protected] > > > > > Subject: [PATCH] add predefined macros for thumbv8 > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > Attached patch adds 2 predefined macros to thumbv8 target: > > > > > > > > > > __thumb2__ > > > > > > > > > > __ __THUMB_INTERWORK__ > > > > > > > > > > > > > > > > > > > > See http://llvm.org/bugs/show_bug.cgi?id=18465 > > > > > > > > > > > > > > > > > > > > Please help to review it. > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > Weiming > > > > > > > > > > > > > > > > > > > > Qualcomm Innovation Center, Inc. is a member of Code Aurora > > Forum, > > > > hosted > > > > > by > > > > > The Linux Foundation > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
