[PATCH] D66969: Output XCOFF object text section

[Digger via Phabricator via llvm-commits]

DiggerLin marked 3 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:354
+  // TODO FIXME How to assert a symbol's visibilty is default?
+  // TODO  Set the function indicator (bit 10, 0x0020) for functions
+  // when debugging is enabled.
----------------
sfertile wrote:
> Minor nit: Extra space after  `TODO`
changed


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:490
   // section header table.
-  uint32_t Address = 0;
+  uint32_t SectionStartAddress = 0;
   // Section indices are 1-based in XCOFF.
----------------
sfertile wrote:
> DiggerLin wrote:
> > sfertile wrote:
> > > Why did you change this, then add an 'Address' in each of the section layout calculations? We can use `Text.Address`/`BSS.Address` to calculate the sections size, so no need for an extra variable.
> > there is a patch https://reviews.llvm.org/D67125 , which will change Address to SectionStartAddress.  And we think SectionStartAddress can express meaning than Address.  
> > 
> > What we need  another available Address in the uint32_t Address = SectionStartAddress; , the Address is used to store the start address of the Csection. 
> > 
> > there is a more variable "Address" as your pointout, but I think the code will more readable with the additional  variable "Address"
> I don't see how the change helps readability. Originally we have a single variable that represent the next assignable address in the file. When we assign a section start address it simply starts at that next available address. After the change we now have a variable to track the `SectionStartAddress`, which doesn't seem any different or special compared to any of the other addresses we assign. And we have the extra overhead of creating a local and assigning that with the value of section start address, then modifying that in the loop making sure we have to copy its modified value out to `SectionStartAddress` at the end of the loop. Why is the sections start address different from any other address we assign in the loop? In what way does having the extra variable increase readability? 
> 
changed as suggestion.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-return55.ll:7
+  ret i32 55
+; CHECK-LABEL: foo
+; CHECK: li 3, 55
----------------
sfertile wrote:
> DiggerLin wrote:
> > hubert.reinterpretcast wrote:
> > > `foo:` with the colon. `.foo:` if possible.
> > changed .thanks
> Still missing the `:` in the CHECK-LABEL line.
added


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66969