[PATCH] add predefined macros for thumbv8
Bernie Ogden
bogden at arm.com
Wed Jan 22 04:32:01 PST 2014
Oops - copying cfe-commits in.
(Still LGTM)
> -----Original Message-----
> From: Bernard Ogden
> Sent: 22 January 2014 12:30
> To: weimingz at codeaurora.org
> Cc: cfe-commits at cs.uiuc.edu
> Subject: RE: [PATCH] add predefined macros for thumbv8
>
> LGTM.
>
> Thanks for all the tweaks.
>
> Regards,
>
> Bernie
>
> > -----Original Message-----
> > From: Weiming Zhao [mailto:weimingz at codeaurora.org]
> > Sent: 21 January 2014 18:55
> > To: Bernard Ogden
> > Cc: cfe-commits at cs.uiuc.edu
> > 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:bogden at arm.com]
> > Sent: Tuesday, January 21, 2014 12:44 AM
> > To: weimingz at codeaurora.org
> > Cc: cfe-commits at cs.uiuc.edu
> > 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: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
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > >
> > >
> >
> >
More information about the cfe-commits
mailing list