scott.linder added a comment.
Sorry for the delay in review, I am not too familiar with XCOFF so I was hoping
someone else would take a look first.
If my understanding of the COFF docs I could find is correct then this LGTM
save for some nits/suggestions
================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:970-1037
+void MCAsmStreamer::emitXCOFFCInfoSym(StringRef Name, StringRef Metadata) {
+ const char *InfoDirective = "\t.info ";
+
+ // Start by emitting the .info pseudo-op and C_INFO symbol name
+ OS << InfoDirective;
+ PrintQuotedString(Name, OS);
+ OS << ", ";
----------------
I sketched out some of the suggestions I had, although the bigger changes are
just because the extra `.info` for the padded byte bugged me. If you aren't
concerned with it I'm also happy with what you have
================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:980
+ size_t MetadataSize = Metadata.size();
+ uint32_t MetadataPaddingSize = 3 - (MetadataSize - 1) % 4;
+
----------------
I couldn't quickly find a reference for the alignment requirement, but it seems
like there is an additional requirement that the length must also be non-zero,
not just even?
If so, can you update the comment?
I would also rather explicitly use `alignTo` and `max` to express this (see
suggestion), but if we don't expect the optimizer to clean it up I'm fine with
the more terse version.
================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:980
+ size_t MetadataSize = Metadata.size();
+ uint32_t MetadataPaddingSize = 3 - (MetadataSize - 1) % 4;
+
----------------
scott.linder wrote:
> I couldn't quickly find a reference for the alignment requirement, but it
> seems like there is an additional requirement that the length must also be
> non-zero, not just even?
>
> If so, can you update the comment?
>
> I would also rather explicitly use `alignTo` and `max` to express this (see
> suggestion), but if we don't expect the optimizer to clean it up I'm fine
> with the more terse version.
Can you factor this out at function scope? It gets repeated below
================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:984
+ uint32_t Length = MetadataSize + MetadataPaddingSize;
+ OS << format_hex(uint32_t(Length), 10) << ",";
+ EmitEOL();
----------------
Redundant cast
================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:988
+ // Return the remaining bytes padded with 0s.
+ auto GetLastWord = [](const uint8_t *Data,
+ uint32_t PaddingBytes) -> uint32_t {
----------------
It seems odd to use a lambda when this is only used once. Why not just compute
the last word directly?
================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:1033
+ MetadataPaddingSize);
+ OS << InfoDirective << ", ";
+ OS << format_hex(LastWord, 10);
----------------
Can you factor this out as `Separator` at function scope?
================
Comment at: llvm/test/CodeGen/PowerPC/aix-command-line-metadata.ll:21
+; Trailing padding:
+; ASM: .info , 0x32330000
+
----------------
Having the padded byte force a new `.info` directive doesn't seem ideal. I
suggested something to avoid it, but I suppose it also doesn't really harm
anything.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153600/new/
https://reviews.llvm.org/D153600
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
