LGTM! Regards,
Bernie > -----Original Message----- > From: Silviu Baranga > Sent: 18 October 2013 15:39 > To: Bernard Ogden; [email protected] > 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; [email protected] > > 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; [email protected] > > > > 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:[email protected]] > > > > > Sent: 18 October 2013 13:07 > > > > > To: Silviu Baranga; [email protected] > > > > > 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: [email protected] [mailto:cfe-commits- > > > > > > [email protected]] On Behalf Of Silviu Baranga > > > > > > Sent: 17 October 2013 16:02 > > > > > > To: [email protected] > > > > > > 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 > > > > > > > > > > > > > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
