This is an archive of the discontinued LLVM Phabricator instance.

[z/OS] Add binary format goff and operating system zos to the triple
ClosedPublic

Authored by Kai on Jun 18 2020, 4:08 AM.

Details

Summary

Adds the binary format goff and the operating system zos to the triple class. goff is selected as default binary format if zos is choosen as operating system. No further functionality is added.

Diff Detail

Event Timeline

Kai created this revision.Jun 18 2020, 4:08 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 18 2020, 4:08 AM
clang/lib/CodeGen/BackendUtil.cpp
258

Minor nit: GOFF appears after XCOFF in most of the "unsorted" lists.

llvm/include/llvm/Support/TargetRegistry.h
520

The coding guidelines have been updated to clarify the formatting of report_fatal_error messages:
https://llvm.org/docs/CodingStandards.html#id14

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
4731

Minor nit: GOFF appears after XCOFF in most of the "unsorted" lists.

4750

Minor nit: GOFF appears after XCOFF in most of the "unsorted" lists.

llvm/lib/Support/Triple.cpp
659

Minor nit: Maintain alphabetical order for lists that are already sorted.

727

Is it beneficial to express the check this way instead of with isOSzOS?

@Kai, I'm afraid I won't be able to assist much in reviews of most z/OS related patches (due to lack of time and lack of expertise with various parts of z/OS), but please do copy me on anything related to source and execution character set encoding or conversions, handling of universal-character-names, and encoding of output files (preprocessor output, dependency output, etc...)

Kai updated this revision to Diff 272010.Jun 19 2020, 4:36 AM
  • Relative order is now consistently GOFF before XCOFF
  • llvm_report_fatal() now follows guideline
  • Updated formatting
Kai marked 4 inline comments as done.Jun 19 2020, 4:39 AM
Kai added inline comments.
llvm/include/llvm/Support/TargetRegistry.h
520

Thanks! That was a good hint.

Kai updated this revision to Diff 272023.Jun 19 2020, 5:15 AM
  • In Triple.cpp: Keep list of binary formats sorted
  • In Triple.cpp: Use isOSzOS() for check
Kai marked 2 inline comments as done.Jun 19 2020, 5:16 AM

LGTM with minor nits.

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
4732

Minor nit: break after a function that does not return appears elsewhere in this switch; please add a break here.

4751

Minor nit: break after a function that does not return appears elsewhere in this switch; please add a break here.

llvm/lib/MC/MCObjectFileInfo.cpp
908

Minor nit: break after a function that does not return appears elsewhere in this switch; please add a break here.

This revision is now accepted and ready to land.Jun 19 2020, 7:37 AM
Kai added a comment.Jun 22 2020, 1:35 AM

I would like to get a 2nd opinion before committing.

MaskRay added inline comments.Jun 23 2020, 1:28 PM
llvm/lib/Support/Triple.cpp
221

Follow the local style by deleting the newline.

Kai updated this revision to Diff 273004.Jun 24 2020, 5:45 AM
  • Added a break; in several places
  • Follow local formatting style in triple.cpp

+@rsmith for clang side changes.

Kai added a reviewer: compnerd.Jul 9 2020, 4:34 AM
Kai added a comment.Jul 15 2020, 4:58 AM

Is there a comment on the clang part?

Still looking for opinions on the clang part!

This revision now requires review to proceed.Aug 7 2020, 9:05 AM

Added the tie breaker when progress cannot be made:)

lattner resigned from this revision.Aug 7 2020, 9:13 AM
This revision is now accepted and ready to land.Aug 7 2020, 9:13 AM

I confirm that I have reviewed the Clang part of this change, which is entirely consistent with the status quo for XCOFF. I'm not seeing any outstanding comments, and I do not believe that the changes are controversial. I think this patch meets the "likely community consensus" requirements for being committed.

tahonermann accepted this revision.Aug 7 2020, 9:29 AM

I'm not a regular Clang reviewer. But for what it is worth, the changes look correct, clear, and appropriate from my lens (though I disagree slightly with some of the lint recommendations in the cases where the recommendation deviates from the surrounding code style). It looks like the concerns Hubert raised have been addressed.

efriedma accepted this revision.Aug 7 2020, 11:54 AM
efriedma added a subscriber: efriedma.

clang changes look fine.