[PATCH] D96184: [AIX][TLS] Generate TLS variables in assembly files

[Victor Huang via Phabricator via llvm-commits]

NeHuang marked an inline comment as done.
NeHuang added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:2248
 
+  if (Kind.isThreadLocal()) {
+    // If data sections are enabled, we emit thread data into separate sections.
----------------
daltenty wrote:
> We should have another case for `Kind.isThreadBSS`, where we should probably use either a csect with XMC_UL or `TLSBSSSection` depending on `TM.getDataSections()`
Agree, we should add the coverage for non-initialized cases using `Kind.isThreadBSS()`
Should we use `Kind.isThreadData()` check instead of `Kind.isThreadLocal()` for the initialized cases?


================
Comment at: llvm/lib/MC/MCSectionXCOFF.cpp:71
+  if (getKind().isBSSLocal() || getKind().isCommon() ||
+      getKind().isThreadBSS()) { // Local uninitialized TLS data.
     assert((getMappingClass() == XCOFF::XMC_RW ||
----------------
We would also need `isEligibleForCommonCsect` check to be aligned here. 


================
Comment at: llvm/lib/MC/MCSectionXCOFF.cpp:83
     return;
   }
 
----------------
We need to cover the other TLS scenario here which is not eligible for commonCsect - to print `CsectDirective`.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-tls-variables.ll:111
+; NODATASEC-NEXT:tls7:
+; NODATASEC-NEXT:  .vbyte  4, 1
+
----------------
lei wrote:
> it would be easier to verify these checks if they were placed right before the declaration of the variable they are checking.  Similar to what is done when we write checks for multiple functions.
Sure. Will also alternate the test cases as below 
`non-tls variable`
`tls variable`
...
...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96184