[PATCH] D69112: [NFC][XCOFF][AIX] Serialize object file writing for each CsectGroup

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:82
+  enum LabelDefinitionUsage : bool {
+    UseLabelDefinition = true,
+    DoNotUseLabelDefinition = false
----------------
Minor nit: 
We have a 2 stage problem when deciding to use label defs or not:
Global: Does the type of csect allow/support labels defs.
Local: If label defs are supported in the csect type, can we omit the label defs for this specific csect.

I believe right now we are always emitting a label def for csects that support them even if it could be skipped, however there has been discussion to skip them when appropriate.  `UseLabelDefinition` to me sounds like its saying implying yes to both the global and local criteria. In that light maybe enum values of 'LabelDefSupported' and 'LabelDefUnsupported'  are more appropriate.  (Or we can adjust the names when we implement the selective label def emission).


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:165
 
-  // CsectGroups. These store the csects which make up different parts of
-  // the sections. Should have one for each set of csects that get mapped into
-  // the same section and get handled in a 'similar' way.
-  CsectGroup ProgramCodeCsects;
-  CsectGroup BSSCsects;
+  // The non-empty sections, in the order they will appear in the section header
+  // table.
----------------
Minor nit: IIUC this is all the sections, as opposed to the non-empty sections. We can determine if a section is non-empty when it has been assigned a valid section index.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:328
+
+    uint32_t CurrentAddressLocation = Section->Address;
+    for (const auto *Group : Section->Groups) {
----------------
Should we  maintain `CurrentAddressLocation` across groups to make sure there is no padding between groups?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:545
+  for (auto *Section : Sections) {
+    for (auto *Group : Section->Groups) {
+      if (Group->Csects.empty())
----------------
I think we should consider using `llvm::all_of` to check if all the groups are empty and continue if they are. Then the section initialization can be done before processing any of the csects, without having to gate the initialization on if its section index is already valid , as well as adjust the size after processing the csects without an addition check to see if the section was initialized or not.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:572
+      if (Section->Index == Section::UninitializedIndex) {
+        assert(SectionIndex <= INT16_MAX && "Section index overflow!");
+        Section->Index = SectionIndex++;
----------------
I think a fatal error would be more appropriate.

Minor nit: should we be creating a named constant for readability/documentation.


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

https://reviews.llvm.org/D69112