This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ][z/OS] z/OS ADA codegen and emission
ClosedPublic

Authored by yusra.syeda on Jun 26 2023, 10:17 AM.

Details

Summary

This patch adds support for the ADA (associated data area), doing the following:

  • Creates the ADA table to handle displacements
  • Emits the ADA section in the SystemZAsmPrinter
  • Lowers the ADA_ENTRY node into the appropriate load instruction

Diff Detail

Event Timeline

yusra.syeda created this revision.Jun 26 2023, 10:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2023, 10:17 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
yusra.syeda requested review of this revision.Jun 26 2023, 10:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2023, 10:17 AM
yusra.syeda edited reviewers, added: Everybody0523; removed: neumannh.Jun 26 2023, 10:18 AM
uweigand accepted this revision.Jun 27 2023, 1:37 AM

LGTM, thanks!

This revision is now accepted and ready to land.Jun 27 2023, 1:37 AM
This revision was landed with ongoing or failed builds.Jun 28 2023, 7:14 AM
This revision was automatically updated to reflect the committed changes.

Updated this diff to fix test failure found in llvm-clang-x86_64-expensive-checks-debian: https://lab.llvm.org/buildbot/#/builders/16/builds/50533

uweigand added inline comments.Jun 30 2023, 7:32 AM
llvm/lib/MC/MCObjectFileInfo.cpp
252

Why are you changing anything about MachO in this patch?

llvm/lib/Target/SystemZ/SystemZOperands.td
664 ↗(On Diff #535942)

Please move this into SystemZInstrInfo.td next to the place where it is used - just like it is done for tlssym.

Also, we're not current using iPTR anywhere in the SystemZ back-end and should not start just for this. Simply use i64.

llvm/lib/Target/SystemZ/SystemZSubtarget.cpp
120

This comment should stay with the "if" below that actually refers to the datalayout.

123

Factoring this out into a separate routine looks like an unrelated change - I guess I'd be fine with this either way.

Change iPTR to i64 and move a comment

yusra.syeda added inline comments.Jun 30 2023, 8:27 AM
llvm/lib/MC/MCObjectFileInfo.cpp
252

I haven't edited this line and it looks the same to me as main. Did I miss something here?

uweigand reopened this revision.Jun 30 2023, 8:30 AM
uweigand added inline comments.
llvm/lib/MC/MCObjectFileInfo.cpp
252

Huh - Phabricator showed a change here when looking at "changes since last action". Looks like this was a false positive.

This revision is now accepted and ready to land.Jun 30 2023, 8:30 AM
uweigand accepted this revision.Jun 30 2023, 8:31 AM

LGTM, thanks!

This revision was landed with ongoing or failed builds.Jul 5 2023, 10:26 AM
This revision was automatically updated to reflect the committed changes.
chapuni added a subscriber: chapuni.Jul 5 2023, 3:12 PM
chapuni added inline comments.
llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp
971

EmittedBytes is unused in -Asserts.

uweigand added inline comments.Jul 6 2023, 6:58 AM
llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp
971

Right, looks like this needs a (void)EmittedBytes; after the assert. @yusra.syeda can you add this?

yusra.syeda added inline comments.Jul 6 2023, 7:20 AM
llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp
971

Sure, I can add this. Can I commit this directly or create a review for it first?

uweigand added inline comments.Jul 6 2023, 8:07 AM
llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp
971

Just go ahead and commit the change directly. Thanks!