This is an archive of the discontinued LLVM Phabricator instance.

[XCOFF][AIX] Alternative path in EHStreamer for platforms do not have uleb128 support
ClosedPublic

Authored by jasonliu on Nov 24 2020, 1:52 PM.

Details

Summary

Not all system assembler supports .uleb128 label2 - label1 form.
When the target do not support this form, we have to take alternative manual calculation to get the offsets from them.

Diff Detail

Event Timeline

jasonliu created this revision.Nov 24 2020, 1:52 PM
jasonliu requested review of this revision.Nov 24 2020, 1:52 PM
llvm/include/llvm/MC/MCAsmInfo.h
190

This also covers/is used for checking support for .sleb128, no? Perhaps this should be HasLEB128Directives?

llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp
420

I agree that the MCAsmInfo is the correct place to encode the support of LEB128 directives or lack thereof; however, I'm not sure that the code here should be overriding the CallSiteEncoding as opposed to having TargetLoweringObjectFile::Initialize set it based on Ctx.getAsmInfo().

llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp
509

s/of/to/; to express the actual relationship.

511

Minor nit: grammar.

513

Minor nit: no comma for two-element list.

517

Minor nit: grammar.

520

Since AIX has a .uleb128 directive, just not one that is fully flexible:
s/.uleb128 directive/general LEB128 directive support/;

525

Minor nit: this is a comparison on an index, this < can be used.

527

Minor nit: grammar.

528

The DWARF specification is consistent in using all caps for the abbreviation of "little-endian base 128".

529

Consider implementing overflow checking here.

llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp
537–540

Range-based for should work here.

541

Minor nit: grammar, style.

551

Minor nit: style.

llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp
559

Use LPStartEncoding and TypeTableEncoding to match the comments or "LPStart Encoding" and "TType Encoding" to match the assembly annotation.

569

Minor nit: put "needs" in scare quotes because we are using potentially-overlong ULEB128 encodings.

570

Typo fix.

572

Minor nit: Use -- or -=.

llvm/lib/MC/MCAsmInfo.cpp
34

s/uleb128 directive/LEB128 directives/;

llvm/lib/MC/MCAsmInfoXCOFF.cpp
26

This makes it so that overriding the default with the option to enable use of LEB128 directives on AIX is not possible. Being able to enable such use is meaningful in the context of the integrated assembler or when using alternate or future AIX assemblers.

llvm/test/CodeGen/PowerPC/aix-exception.ll
101–102

Why the shift here? The labels were aligned before.

110–113

Be consistent on the use of physical tab characters (and space characters preceding such to maintain display alignment).

146–148

Same comment re: why the shift?

llvm/lib/MC/MCAsmInfo.cpp
31–32

Avoid option naming that only covers the "unsigned" case.

jasonliu marked an inline comment as done.Dec 1 2020, 11:00 AM
jasonliu added inline comments.Dec 1 2020, 11:29 AM
llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp
420

TargetLoweringObjectFile get inheritance a lot, so setting it there means we need to override it in multiply places. And when a new TargetLoweringObjectFile derived class is created, and they might also messes the setting for CallSiteEncoding. I'm contemplating about overriding it in TargetLoweringObjectFile::getCallSiteEncoding() call instead. What do you think?

llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp
420

I think that's a reasonable choice.

jasonliu updated this revision to Diff 308742.Dec 1 2020, 12:45 PM
jasonliu marked 21 inline comments as done.

Address comments.

jasonliu updated this revision to Diff 308744.Dec 1 2020, 12:51 PM

LGTM with comments.

llvm/include/llvm/MC/MCAsmInfo.h
189

Minor nit: missing "the".

llvm/lib/MC/MCAsmInfo.cpp
32

Fix typo. Test uses use-leb128-directives as intended.

llvm/lib/MC/MCAsmInfoXCOFF.cpp
26–28

The value to use if not unset is already handled.

llvm/lib/Target/TargetLoweringObjectFile.cpp
60

Minor nit: style.

62

Same comment.

This revision is now accepted and ready to land.Dec 1 2020, 1:40 PM
jasonliu updated this revision to Diff 308968.Dec 2 2020, 7:31 AM
jasonliu marked 5 inline comments as done.

Address comments and rebase.

Confirming LGTM.

jasonliu closed this revision.Dec 2 2020, 12:32 PM

Manually closing this because I spelled the Differential Revision wrong.