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

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:82
+  enum LabelDefinitionUsage : bool {
+    UseLabelDefinition = true,
+    DoNotUseLabelDefinition = false
----------------
sfertile wrote:
> 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).
Agree. 


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:328
+
+    uint32_t CurrentAddressLocation = Section->Address;
+    for (const auto *Group : Section->Groups) {
----------------
sfertile wrote:
> Should we  maintain `CurrentAddressLocation` across groups to make sure there is no padding between groups?
Do you mean maintain CurrentAddressLocation across Sections? Since it's currently already maintained across groups. 
Made the changes based on my interpretation. 


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:545
+  for (auto *Section : Sections) {
+    for (auto *Group : Section->Groups) {
+      if (Group->Csects.empty())
----------------
sfertile wrote:
> 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.
I still need a guard for 
Section->Address = Group->Csects.front().Address;

But I do think using a specific guard for that is better than just comparing UninitializedIndex everywhere. 
Thanks.


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

https://reviews.llvm.org/D69112