[PATCH] Enable the __ARM_ARCH_EXT_IDIV__ predefine generation on ARM

[Bernie Ogden]

LGTM!

Regards,

Bernie

> -----Original Message-----
> From: Silviu Baranga
> Sent: 18 October 2013 15:39
> To: Bernard Ogden; cfe-commits at cs.uiuc.edu
> Subject: RE: [PATCH] Enable the __ARM_ARCH_EXT_IDIV__ predefine
> generation on ARM
> 
> Hi Bernie,
> 
> I'm attaching a new set of patches.
> 
> Changes:
> clang_ext.diff:
>   - removed the added checks from the ABI tests. They were not
>     adding any value.
> 
> clang_defaults.diff:
>   - we are now testing that the arch is armv8, armv8a, thumbv8 or
>     thumbv8a to turn set the features as defaults.
>     This should not match a theoretical armv8r for example, although
>     that is impossible to test. Also added some new tests for this.
>   - added the nop tests (under the cortex-a15 ones with labels
>     ARMHWDIV-ARM and THUMBHWDIV-THUMB) - I think these tests are
>     worth it.
> 
> Thanks,
> Silviu
> 
> 
> 
> > -----Original Message-----
> > From: Bernard Ogden
> > Sent: 18 October 2013 14:18
> > To: Silviu Baranga; cfe-commits at cs.uiuc.edu
> > Subject: RE: [PATCH] Enable the __ARM_ARCH_EXT_IDIV__ predefine
> > generation on ARM
> >
> > > No, this test isn't related to the ABIs, but to the fact that the
> > > triple starts with arm- (so we don't know if we have hardware
> > > division available). Since the check is relatively inexpensive,
> > > I think it's worth doing (although the ARMEABI* checks seem to test
> > > the ABI).
> >
> > If these tests are specifically about the ABI then I wouldn't add a
> > non-ABI
> > test to them - it's confusing. I think arm-target-features.c would be
> a
> > better place for that test.
> >
> >
> > > > > (Targets.cpp): I'm a bit edgy about the check for triples
> > starting
> > > > with
> > > > > armv8. What if some future profile does not have idiv in some
> > > > > instrution set? Will it still work if you change that to check
> > for
> > > > > armv8a?
> > > My reasoning was that the two are currently the same, but I would
> > like
> > > to avoid changing the behaviour in the future (so I'll change it to
> > > check
> > > for armv8a, at least for now).
> >
> > I expect that armv8- will always mean armv8a. But I think the test as
> > written will also match armv8anything_else.
> >
> > So, depending on what canonicalisation is or isn't happening, you
> might
> > need
> > to check for both armv8- and armv8a.
> >
> >
> > > > > (arm-target-features.c): Is it worth adding tests that the
> option
> > > is
> > > > a
> > > > > nop when it should be (e.g. -target arm -mthumb -mhwdiv=thumb)?
> > > The option shouldn't be a nop there. The -target arm -mthumb should
> > be
> > > equivalent to "-cc1 -triple thumb" and it won't have hardware
> > division
> > > on by default.
> >
> > True, but I think the principle still applies. What about -target
> armv8
> > -mthumb -mhwdiv=thumb (which is a nop at least in terms of whether
> the
> > predefine gets set). I'm not sure whether we _need_ to test the nop
> > case,
> > but I think it exists - just wondered whether you think it's worth
> > testing.
> >
> > Regards,
> >
> > Bernie
> >
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Bernard Ogden
> > > > Sent: 18 October 2013 13:23
> > > > To: Silviu Baranga; cfe-commits at cs.uiuc.edu
> > > > Subject: RE: [PATCH] Enable the __ARM_ARCH_EXT_IDIV__ predefine
> > > > generation on ARM
> > > >
> > > > Actually, ignore my first comment. There is a scenario (v7A
> without
> > > > virtualization) where we could have idiv in ARM but not Thumb.
> > > >
> > > > > -----Original Message-----
> > > > > From: Bernie Ogden [mailto:bogden at arm.com]
> > > > > Sent: 18 October 2013 13:07
> > > > > To: Silviu Baranga; cfe-commits at cs.uiuc.edu
> > > > > Subject: RE: [PATCH] Enable the __ARM_ARCH_EXT_IDIV__ predefine
> > > > > generation on ARM
> > > > >
> > > > > Hi Siviu,
> > > > >
> > > > > clang_ext.diff:
> > > > >
> > > > > In principle there should be no targets where we have idiv in
> ARM
> > > but
> > > > > not Thumb - but I'm happy with the option as-is because it lets
> > the
> > > > > user choose to not generate the instructions per-instruction-
> set
> > > > should
> > > > > he ever want to. Who are we to declare what users should do? ;)
> > > > >
> > > > > (init.c) I guess that we don't want the predefine in these
> tests
> > > > > because the target is Linux, rather than anything to do with
> the
> > > > float
> > > > > ABIs? If so, is the right place for that test?
> > > > >
> > > > >
> > > > > clang_defaults.diff:
> > > > >
> > > > > (Targets.cpp): I'm a bit edgy about the check for triples
> > starting
> > > > with
> > > > > armv8. What if some future profile does not have idiv in some
> > > > > instrution set? Will it still work if you change that to check
> > for
> > > > > armv8a?
> > > > >
> > > > > (arm-target-features.c): Is it worth adding tests that the
> option
> > > is
> > > > a
> > > > > nop when it should be (e.g. -target arm -mthumb -mhwdiv=thumb)?
> > > > >
> > > > >
> > > > > Regards,
> > > > >
> > > > > Bernie
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: cfe-commits-bounces at cs.uiuc.edu [mailto:cfe-commits-
> > > > > > bounces at cs.uiuc.edu] On Behalf Of Silviu Baranga
> > > > > > Sent: 17 October 2013 16:02
> > > > > > To: cfe-commits at cs.uiuc.edu
> > > > > > Subject: [PATCH] Enable the __ARM_ARCH_EXT_IDIV__ predefine
> > > > > generation
> > > > > > on ARM
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > The following set of patches enable the generation of the
> > > > > > __ARM_ARCH_EXT_IDIV__ predefine
> > > > > > for ARM targets which support division instructions. This
> will
> > > fix
> > > > > > PR17555.
> > > > > >
> > > > > > clang_ext.diff:
> > > > > > Targets having the hwdiv or hwdiv-arm feature will also the
> > > > > > __ARM_ARCH_EXT_IDIV__ predefine.
> > > > > >
> > > > > > Also adds the -mhwdiv option to modify these features. The
> > > > arguments
> > > > > to
> > > > > > this option
> > > > > > can be arm/thumb/arm,thumb and indicate in which mode the
> > > hardware
> > > > > > division instructions
> > > > > > are supported (for example passing -mhwdiv=arm means the
> target
> > > > > support
> > > > > > hardware division
> > > > > > instructions in arm mode but not in thumb mode).
> > > > > >
> > > > > > clang_defaults.diff:
> > > > > > Depends on clang_ext.diff.
> > > > > >
> > > > > > Sets the default hwdiv/hwdiv-arm features for different
> > targets.
> > > > The
> > > > > > feature is added by
> > > > > > default for all AArch32 v8 targets, Cortex-A7, Cortex-A15,
> > swift,
> > > > > > Cortex-A53, Cortex-A57,
> > > > > > Cortex-R5, Cortex-M4 and Cortex-M3.
> > > > > >
> > > > > > Please review!
> > > > > >
> > > > > > Thanks,
> > > > > > Silviu
> > > > > >
> > > > > > -- IMPORTANT NOTICE: The contents of this email and any
> > > attachments
> > > > > are
> > > > > > confidential and may also be privileged. If you are not the
> > > > intended
> > > > > > recipient, please notify the sender immediately and do not
> > > disclose
> > > > > the
> > > > > > contents to any other person, use it for any purpose, or
> store
> > or
> > > > > copy
> > > > > > the information in any medium.  Thank you.
> > > > > >
> > > > > > ARM Limited, Registered office 110 Fulbourn Road, Cambridge
> CB1
> > > > 9NJ,
> > > > > > Registered in England & Wales, Company No:  2557590
> > > > > > ARM Holdings plc, Registered office 110 Fulbourn Road,
> > Cambridge
> > > > CB1
> > > > > > 9NJ, Registered in England & Wales, Company No:  2548782
> > > >
> > >
> > >
> >
> >