Page MenuHomePhabricator

[AArch64][AsmParser] Remove 'x31' alias for 'sp/xzr' register.
ClosedPublic

Authored by CarolineConcatto on Mon, Oct 26, 6:50 AM.

Details

Summary

Only the aliases 'xzr' and 'sp' exist for the physical register x31.
The reason for wanting to remove the alias 'x31' is because it allows users
to write invalid asm that is not accepted by the GNU assembler.

Is there any objection to removing this alias? Or do we want to keep
this for compatibility with existing code that uses w31/x31?

Diff Detail

Event Timeline

CarolineConcatto requested review of this revision.Mon, Oct 26, 6:50 AM

If this was allowed:

ldrb	w0, [sp, x31]

but disallowed with this change, then this is a breaking change that doesn't seem reasonable.

The reason for wanting to remove the alias 'x31' is because it allows users to write invalid asm that is not accepted by the GNU assembler.

So, why is this not allowed by the gnu assembler?

ostannard accepted this revision.Tue, Oct 27, 2:26 AM

Using x31 is confusing and not compatible with the GNU assembler, which are both god reasons to reject it. Also, the only occurrence of x31 in the architecture manual is this:

There is no register named W31 or X31.

This revision is now accepted and ready to land.Tue, Oct 27, 2:26 AM

I don't know what route is normally taken in these situations; this change would indeed break code that relies on this 'feature' even though removing parsing support for w31/x31 is correct. An alternative approach would be to emit a warning diagnostic that says the use of w31/x31 is deprecated and that support will be removed in a future release. Is that an approach we want to take, or is it fine to just remove support for it?

I'm not aware of any official policy about this, but I don't think it's worth going through a multi-release deprecation process for something which is called out in the architecture manual as not valid.

  • update the release note page on llvm
sdesmalen added inline comments.Thu, Oct 29, 11:08 AM
llvm/docs/ReleaseNotes.rst
98

How about simply:

The assembler no longer accepts ``w31`` and ``x31`` as aliases for ``wzr`` and ``xzr``, because the architecture manual explicitly states that no registers with those names exist.

?

-update message on the release note for llvm

CarolineConcatto marked an inline comment as done.Thu, Oct 29, 11:16 AM
sdesmalen accepted this revision.Thu, Oct 29, 11:20 AM

Thanks for adding the note, LGTM!

I'll be the dissenting voice: I don't think this is the right approach. "It's what gas does" is not a sufficient reason to break user code. I think a warning and *possibly* removing support some time well in the future is the right solution here.

Keep in mind that just because the architecture manual currently says that the name doesn't exist does not mean that it always did. I'm personally aware of a good bit of aarch64 assembly that was written well before the architecture manual was finalized, and there are definitely instances where the specification of the asm syntax was clarified in ways that are incompatible with how it was original stated.

This revision was landed with ongoing or failed builds.Sun, Nov 1, 11:58 PM
This revision was automatically updated to reflect the committed changes.

@CarolineConcatto I don't think this patch should have landed because @resistor made some valid arguments on why not to remove support for this directly, but rather to take the approach of emitting a warning instead. Can you please revert the patch?