On Thu, Oct 26, 2023 at 08:31:04PM +0100, Iain Sandoe wrote: > This is an enablement patch (the initial use comes with the Darwin aarch64 > port). Tested on aarch64-linux-gnu, aarch64-apple-darwin and x86_64-darwin > just for good measure, OK for trunk? > thanks > Iain. > > --- 8< --- > > Some assmblers have a bug that requires +crc to be emitted even > though the base architecture supports it. However, that also > triggers a different bug in another assembler. So make the fix > configurable.
I only just noticed this, sorry, but I don't think it's as straightforward as "triggers a different bug in another assembler". It's actually "avoids triggering one bug in another assembler, which means we hit a different bug in that assembler more often". I described the bugs in detail in https://gcc.gnu.org/pipermail/gcc-patches/2025-January/674446.html. There's no sensible way to workaround both of those bugs at the same time. The effect of this patch is that some .arch directives will be ignored, instead of being applied with missing features. In some cases this helps (if the .arch directive includes unrecognised feature names that will be incorrectly dropped, and there are no recognised optional features specified). In other cases (likely much rarer), this patch makes things worse (if the .arch directive specifies a higher architecture version, with only the mandatory architecture features present). Given that the broken Binutils versions don't support Darwin, I think it's fine to disable the CRC workaround when targetting Darwin, or if we otherwise know that we're using an old LLVM version for assembly. This will improve the likelihood of being able to assemble the resulting output with LLVM <=16, but it's still going to break sometimes, and realistically the best option is to avoid using LLVM <=16 (or other assemblers without the LLVM 17 fixes) for assembly if at all possible. To be clear - I have no objection to this patch remaining in (or being replaced with a patch that disables the workaround for Darwin), but I wanted to make clear that the situation is more nuanced than your commit message suggests. Alice > > gcc/ChangeLog: > > * common/config/aarch64/aarch64-common.cc: Make the asm > crc bug workaround configurable. > > Signed-off-by: Iain Sandoe <[email protected]> > --- > gcc/common/config/aarch64/aarch64-common.cc | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/gcc/common/config/aarch64/aarch64-common.cc > b/gcc/common/config/aarch64/aarch64-common.cc > index 20bc4e1291b..4922a6b235c 100644 > --- a/gcc/common/config/aarch64/aarch64-common.cc > +++ b/gcc/common/config/aarch64/aarch64-common.cc > @@ -301,8 +301,12 @@ aarch64_get_extension_string_for_isa_flags > > However, assemblers with Armv8-R AArch64 support should not have this > issue, so we don't need this fix when targeting Armv8-R. */ > - auto explicit_flags = (!(current_flags & AARCH64_FL_V8R) > - ? AARCH64_FL_CRC : 0); > + aarch64_feature_flags explicit_flags = > +#ifndef DISABLE_AARCH64_AS_CRC_BUGFIX > + (!(current_flags & AARCH64_ISA_V8R) ? AARCH64_FL_CRC : 0); > +#else > + 0; > +#endif > > /* Add the features in isa_flags & ~current_flags using the smallest > possible number of extensions. We can do this by iterating over the > -- > 2.39.2 (Apple Git-143) >
