Page MenuHomePhabricator

[COFF, ARM64] Use '//' as comment character in assembly files in GNU environments
ClosedPublic

Authored by mstorsjo on Aug 5 2017, 1:06 PM.

Details

Summary

This allows using semicolons for bundling up more than one statement per line. This is used within the mingw-w64 project in some assembly files that contain code for multiple architectures.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Aug 5 2017, 1:06 PM
compnerd requested changes to this revision.Aug 5 2017, 9:34 PM

Please create a special GNU specialization for this.

This revision now requires changes to proceed.Aug 5 2017, 9:34 PM
mstorsjo updated this revision to Diff 109913.Aug 6 2017, 4:06 AM
mstorsjo edited edge metadata.
mstorsjo retitled this revision from [COFF, ARM64] Use '//' as comment character in assembly files to [COFF, ARM64] Use '//' as comment character in assembly files in GNU environments.

Limited the change to GNU environments.

martell added inline comments.Aug 6 2017, 7:45 AM
lib/Target/AArch64/MCTargetDesc/AArch64MCTargetDesc.cpp
72 ↗(On Diff #109913)

Can we make this consistent with the other Targets?
In ARMMCTargetDesc.cpp we have

else if (TheTriple.isWindowsMSVCEnvironment())
  MAI = new ARMCOFFMCAsmInfoMicrosoft();
else if (TheTriple.isOSWindows())
  MAI = new ARMCOFFMCAsmInfoGNU();

So flip this so it checks if MSVCEnvironment first and if not and still windows do the GNU style.
It also seems we should have
AArch64COFFMCAsmInfo and not AArch64MCAsmInfoCOFF but that is outside the scope of this patch

mstorsjo added inline comments.Aug 6 2017, 8:14 AM
lib/Target/AArch64/MCTargetDesc/AArch64MCTargetDesc.cpp
72 ↗(On Diff #109913)

Sure. Then I'll update the existing test in test/MC/AArch64/coff-relocations.s from aarch64-windows to aarch64-windows-msvc, since that test uses ; for comments. Then it might also make sense to add a AArch64COFFMCAsmInfoMicrosoft frontend classs if msvc is the exception, not the default case, right?

martell added inline comments.Aug 6 2017, 8:17 AM
lib/Target/AArch64/MCTargetDesc/AArch64MCTargetDesc.cpp
72 ↗(On Diff #109913)

I never clarified why... cygwin, midipix etc. :)

else if (TheTriple.isWindowsMSVCEnvironment())
  MAI = new AArch64MCAsmInfoCOFF();
else if (TheTriple.isOSWindows())
  MAI = new AArch64MCAsmInfoCOFFGNU();

should suffice, though AArch64MCAsmInfoCOFF should be AArch64COFFMCAsmInfoMicrosoft

martell added inline comments.Aug 6 2017, 8:20 AM
lib/Target/AArch64/MCTargetDesc/AArch64MCTargetDesc.cpp
72 ↗(On Diff #109913)

I wrote that before seeing your reply.
Yeah that probably makes the most sense here.

mstorsjo updated this revision to Diff 109933.Aug 6 2017, 12:44 PM

Made the GNU case the default. Apparently I didn't have to update the existing test either, because aarch64-windows still triggers isWindowsMSVCEnvironment (but not isKnownWindowsMSVCEnvironment).

Ping @compnerd, any further comments on this?

martell edited edge metadata.Aug 11 2017, 6:12 AM

@mstorsjo Shouldn't AArch64MCAsmInfoMicrosoft have the word COFF in it somewhere?

@mstorsjo Shouldn't AArch64MCAsmInfoMicrosoft have the word COFF in it somewhere?

I guess - I can amend it that way later.

mstorsjo updated this revision to Diff 110778.Aug 11 2017, 11:28 AM

Renamed AArch64MCAsmInfoMicrosoft to AArch64MCAsmInfoMicrosoftCOFF.

compnerd accepted this revision.Aug 12 2017, 10:07 PM
This revision is now accepted and ready to land.Aug 12 2017, 10:07 PM
This revision was automatically updated to reflect the committed changes.