[PATCH] D104644: [AIX][XCOFF] Support 64-bit relocation writing and related tests

Tue Jun 22 01:16:53 PDT 2021

jhenderson added a comment.

This patch from the description is about 64-bit relocation support, so I'm not sure why there's so much changing in the testing. You shouldn't need so many tests for ensuring you have 64-bit relocation support, which implies that the existing tests aren't well designed - they are testing things in a too catch-all manner, and should probably be rewritten to test a small subset of functionality each. Furthermore, why does every test need to cover 64-bit behaviour?

================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:61
     report_fatal_error("Unimplemented fixup kind.");
+  case PPC::fixup_ppc_half16ds:
   case PPC::fixup_ppc_half16: {
----------------
I'm not a PPC/XCOFF/AIX person, so I might be talking nonsense, but it seems like this part of the patch (i.e. the file) isn't really about 64-bit relocation support in XCOFF files?

================
Comment at: llvm/test/CodeGen/PowerPC/aix-extern-weak.ll:335
+
+; CHECKSYM64: Symbols [
+; CHECKSYM64-NEXT:   Symbol {
----------------
Nit: Line things up for readability.

That being said, are you able to share these checks with the above set, so that you don't have to have the details listed out twice? If necessary, you could use --check-prefixes to specify multiple prefixes so that you can have common parts, and then separate parts for where the two sets differ.

The same sort of comments apply in the other tests too.

================
Comment at: llvm/test/CodeGen/PowerPC/aix-extern-weak.ll:347
+; CHECKSYM64-NEXT:   Symbol {
+; CHECKSYM64-NEXT:     Index: [[#Index:]]
+; CHECKSYM64-NEXT:     Name: .foo_ext_weak
----------------
If I'm not mistaken, you should capture the .file symbol index, rather than this one and work from there.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104644/new/

https://reviews.llvm.org/D104644