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.
Details
Diff Detail
Event Timeline
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: | |
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 | ||
---|---|---|
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. |
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. |
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. |
Minor nit: missing "the".