[PATCH] Enable the __ARM_ARCH_EXT_IDIV__ predefine generation on ARM

[Bernie Ogden]

> 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
> >
> 
>