Page MenuHomePhabricator

[ARM][AsmParser] Make assembly directives case insensitive
ClosedPublic

Authored by DavidSpickett on Jan 27 2020, 5:56 AM.

Diff Detail

Event Timeline

DavidSpickett created this revision.Jan 27 2020, 5:56 AM
DavidSpickett edited reviewers, added: ostannard; removed: olista01.Jan 27 2020, 6:08 AM
ostannard accepted this revision.Feb 4 2020, 5:58 AM

LGTM with a few nits.

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
10505

Don't use auto here, the type is not obvious.

llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
241 ↗(On Diff #240544)

This is unrelated, should be a separate patch.

This revision is now accepted and ready to land.Feb 4 2020, 5:58 AM
DavidSpickett marked an inline comment as done.Feb 4 2020, 8:37 AM
This revision was automatically updated to reflect the committed changes.
DavidSpickett marked an inline comment as done.Feb 4 2020, 9:11 AM
DavidSpickett edited the summary of this revision. (Show Details)
MaskRay added a subscriber: MaskRay.EditedFeb 4 2020, 1:15 PM

Sorry, but I don't understand the rationale behind this change.

Yes, GNU as accepts .Section and llvm-mc doesn't. There are plenty of cases where MC does not recognize uppercase directives. I don't find this (libgloss/arm/linux-syscalls0.S) compelling enough to make MC accept every case-insensitive directive.
Changing the downstream projects may be more straightforward.

The rationale is that the case of a directive has never really been important even though we'd reject anything that isn't lower case. I looked at all the targets in LLVM and none of them specify anything that isn't all lower case (though nothing in the code prevents them from doing so). All the targets GCC supports will accept any case also, though they may document with a set format.

So yes, we could change projects that don't use lower case. It's presumably a small set because Clang has been around a while and that bugzilla is the only issue I know of.

My point is more, was the case of directives ever actually important or was it just implemented that way? Seems to me like the latter. Though I can't speak for other targets hence I've only changed the generic, ARM and AArch64 directives.

Thinking about it, whatever the behaviour it should be documented (perhaps here: https://llvm.org/docs/Extensions.html). Having alignment across targets would be best, which is something for a mailing list discussion. I will post there and put a link back here so you can reply there.