| Message ID | a97b57ec-04e7-1a90-ce19-ff981bf33181@e124511.cambridge.arm.com |
|---|---|
| Headers | show |
| Series | aarch64: Fix intrinsic availability [PR112108] | expand |
Andrew Carlotti <andrew.carlotti@arm.com> writes: > This series of patches fixes issues with some intrinsics being incorrectly > gated by global target options, instad of just using function-specific target > options. These issues have been present since the +tme, +memtag and +ls64 > intrinsics were introduced. > > Compared to the previous version, this series no longer adds feature checks to > the intrinsic expanders, and fixes various formatting issues pointed out by > Richard Sandiford. > > Additionally, the series now refactors the checking of TARGET_GENERAL_REGS_ONLY > in check_required_extensions. This refactor is included as a new patch (1/5) > to make the diffs more readable. > > > Bootstrapped and regression tested on aarch64. Ok to merge? LGTM, thanks. OK if there are no other comments before the weekend. > Also, ok for backports to affected versions (with regression tests)? Hmm, it seems a bit invasive. And if the GCC 11 tag in the PR is anything to go by, it sounds like this is already unfixable behaviour in at least one release series. Let's see if anyone else has any opinions. Richard
On Thu, Aug 15, 2024 at 05:15:03PM +0100, Richard Sandiford wrote: > Andrew Carlotti <andrew.carlotti@arm.com> writes: > > This series of patches fixes issues with some intrinsics being incorrectly > > gated by global target options, instad of just using function-specific target > > options. These issues have been present since the +tme, +memtag and +ls64 > > intrinsics were introduced. > > > > Compared to the previous version, this series no longer adds feature checks to > > the intrinsic expanders, and fixes various formatting issues pointed out by > > Richard Sandiford. > > > > Additionally, the series now refactors the checking of TARGET_GENERAL_REGS_ONLY > > in check_required_extensions. This refactor is included as a new patch (1/5) > > to make the diffs more readable. > > > > > > Bootstrapped and regression tested on aarch64. Ok to merge? > > LGTM, thanks. OK if there are no other comments before the weekend. > > > Also, ok for backports to affected versions (with regression tests)? > > Hmm, it seems a bit invasive. And if the GCC 11 tag in the PR is > anything to go by, it sounds like this is already unfixable behaviour > in at least one release series. I think the impact is minimal prior to FMV support, so backporting is less important for older versions. The series should backport cleanly to GCC 14, but would have conflicts in earlier version, so I think it would be sensible to backport to GCC 14 and not further. > Let's see if anyone else has any opinions. > > Richard
> On 15 Aug 2024, at 18:48, Andrew Carlotti <andrew.carlotti@arm.com> wrote: > > External email: Use caution opening links or attachments > > > On Thu, Aug 15, 2024 at 05:15:03PM +0100, Richard Sandiford wrote: >> Andrew Carlotti <andrew.carlotti@arm.com> writes: >>> This series of patches fixes issues with some intrinsics being incorrectly >>> gated by global target options, instad of just using function-specific target >>> options. These issues have been present since the +tme, +memtag and +ls64 >>> intrinsics were introduced. >>> >>> Compared to the previous version, this series no longer adds feature checks to >>> the intrinsic expanders, and fixes various formatting issues pointed out by >>> Richard Sandiford. >>> >>> Additionally, the series now refactors the checking of TARGET_GENERAL_REGS_ONLY >>> in check_required_extensions. This refactor is included as a new patch (1/5) >>> to make the diffs more readable. >>> >>> >>> Bootstrapped and regression tested on aarch64. Ok to merge? >> >> LGTM, thanks. OK if there are no other comments before the weekend. >> >>> Also, ok for backports to affected versions (with regression tests)? >> >> Hmm, it seems a bit invasive. And if the GCC 11 tag in the PR is >> anything to go by, it sounds like this is already unfixable behaviour >> in at least one release series. > > I think the impact is minimal prior to FMV support, so backporting is less > important for older versions. The series should backport cleanly to GCC 14, > but would have conflicts in earlier version, so I think it would be sensible to > backport to GCC 14 and not further. I think backporting only to GCC 14 is sensible. The intrinsics in question tbh are or will be shipping hardware that I don’t expect will be used with older compilers much to be worth the risk of adjusting the patches for those branches. Thanks, Kyrill > >> Let's see if anyone else has any opinions. >> >> Richard
On Fri, Aug 16, 2024 at 07:17:24AM +0000, Kyrylo Tkachov wrote: > > > > On 15 Aug 2024, at 18:48, Andrew Carlotti <andrew.carlotti@arm.com> wrote: > > > > External email: Use caution opening links or attachments > > > > > > On Thu, Aug 15, 2024 at 05:15:03PM +0100, Richard Sandiford wrote: > >> Andrew Carlotti <andrew.carlotti@arm.com> writes: > >>> This series of patches fixes issues with some intrinsics being incorrectly > >>> gated by global target options, instad of just using function-specific target > >>> options. These issues have been present since the +tme, +memtag and +ls64 > >>> intrinsics were introduced. > >>> > >>> Compared to the previous version, this series no longer adds feature checks to > >>> the intrinsic expanders, and fixes various formatting issues pointed out by > >>> Richard Sandiford. > >>> > >>> Additionally, the series now refactors the checking of TARGET_GENERAL_REGS_ONLY > >>> in check_required_extensions. This refactor is included as a new patch (1/5) > >>> to make the diffs more readable. > >>> > >>> > >>> Bootstrapped and regression tested on aarch64. Ok to merge? > >> > >> LGTM, thanks. OK if there are no other comments before the weekend. > >> > >>> Also, ok for backports to affected versions (with regression tests)? > >> > >> Hmm, it seems a bit invasive. And if the GCC 11 tag in the PR is > >> anything to go by, it sounds like this is already unfixable behaviour > >> in at least one release series. > > > > I think the impact is minimal prior to FMV support, so backporting is less > > important for older versions. The series should backport cleanly to GCC 14, > > but would have conflicts in earlier version, so I think it would be sensible to > > backport to GCC 14 and not further. > > I think backporting only to GCC 14 is sensible. The intrinsics in question tbh are or will be shipping hardware that I don’t expect will be used with older compilers much to be worth the risk of adjusting the patches for those branches. > Thanks, > Kyrill > > > > > >> Let's see if anyone else has any opinions. > >> > >> Richard > I've pushed this to master now (with a couple of Changelog fixes). I'll backport it to GCC 14 next week if there are no issues.
On Mon, Aug 19, 2024 at 03:52:58PM +0100, Andrew Carlotti wrote: > On Fri, Aug 16, 2024 at 07:17:24AM +0000, Kyrylo Tkachov wrote: > > > > > > > On 15 Aug 2024, at 18:48, Andrew Carlotti <andrew.carlotti@arm.com> wrote: > > > > > > External email: Use caution opening links or attachments > > > > > > > > > On Thu, Aug 15, 2024 at 05:15:03PM +0100, Richard Sandiford wrote: > > >> Andrew Carlotti <andrew.carlotti@arm.com> writes: > > >>> This series of patches fixes issues with some intrinsics being incorrectly > > >>> gated by global target options, instad of just using function-specific target > > >>> options. These issues have been present since the +tme, +memtag and +ls64 > > >>> intrinsics were introduced. > > >>> > > >>> Compared to the previous version, this series no longer adds feature checks to > > >>> the intrinsic expanders, and fixes various formatting issues pointed out by > > >>> Richard Sandiford. > > >>> > > >>> Additionally, the series now refactors the checking of TARGET_GENERAL_REGS_ONLY > > >>> in check_required_extensions. This refactor is included as a new patch (1/5) > > >>> to make the diffs more readable. > > >>> > > >>> > > >>> Bootstrapped and regression tested on aarch64. Ok to merge? > > >> > > >> LGTM, thanks. OK if there are no other comments before the weekend. > > >> > > >>> Also, ok for backports to affected versions (with regression tests)? > > >> > > >> Hmm, it seems a bit invasive. And if the GCC 11 tag in the PR is > > >> anything to go by, it sounds like this is already unfixable behaviour > > >> in at least one release series. > > > > > > I think the impact is minimal prior to FMV support, so backporting is less > > > important for older versions. The series should backport cleanly to GCC 14, > > > but would have conflicts in earlier version, so I think it would be sensible to > > > backport to GCC 14 and not further. > > > > I think backporting only to GCC 14 is sensible. The intrinsics in question tbh are or will be shipping hardware that I don’t expect will be used with older compilers much to be worth the risk of adjusting the patches for those branches. > > Thanks, > > Kyrill > > > > > > > > > >> Let's see if anyone else has any opinions. > > >> > > >> Richard > > > > I've pushed this to master now (with a couple of Changelog fixes). I'll > backport it to GCC 14 next week if there are no issues. Backported cleanly to GCC 14, and pushed after passing regression testing.