[PATCH] add predefined macros for thumbv8

[Bernie Ogden]

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:weimingz at codeaurora.org]
> Sent: 21 January 2014 01:34
> To: Bernard Ogden
> Cc: cfe-commits at cs.uiuc.edu
> 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:bogden at arm.com]
> Sent: Monday, January 20, 2014 4:32 AM
> To: weimingz at codeaurora.org
> Cc: cfe-commits at cs.uiuc.edu
> 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: weimingz at codeaurora.org [mailto:weimingz at codeaurora.org]
> > Sent: 20 January 2014 01:03
> > To: Bernard Ogden
> > Cc: weimingz at codeaurora.org; cfe-commits at cs.uiuc.edu
> > 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:weimingz at codeaurora.org]
> > > Sent: 15 January 2014 22:49
> > > To: Bernard Ogden; cfe-commits at cs.uiuc.edu
> > > 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:bogden at arm.com]
> > > Sent: Wednesday, January 15, 2014 4:22 AM
> > > To: weimingz at codeaurora.org; cfe-commits at cs.uiuc.edu
> > > 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: cfe-commits-bounces at cs.uiuc.edu
> > > [mailto:cfe-commits-bounces at cs.uiuc.edu] On Behalf Of Weiming Zhao
> > > Sent: 14 January 2014 01:03
> > > To: cfe-commits at cs.uiuc.edu
> > > 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
> > >
> > >
> > >
> >
> >
> 
>