[PATCH] D28150: Move the section name from GlobalObject to the LLVMContext

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked 2 inline comments as done.
rnk added inline comments.


================
Comment at: include/llvm/IR/GlobalObject.h:41
-  std::string Section;     // Section to emit this into, empty means default
   Comdat *ObjComdat;
   enum {
----------------
mehdi_amini wrote:
> We could do the same for the comdat?
I'd want to do performance measurements before doing that. For C++ targeting not MachO, basically everything has a comdat, and I don't want to regress that. I'm aware that nothing has a comdat on MachO, but it's something we'd want to measure before changing.

Also, the wins are much smaller. std::string is usually 3 pointers, vs one for comdat.


================
Comment at: include/llvm/IR/GlobalObject.h:72
+  }
+  StringRef getSection() const {
+    return hasSection() ? getSectionImpl() : StringRef();
----------------
mehdi_amini wrote:
> I'd comment here that the access is not efficient and client is advise to check for `hasSection()` before calling it.
Done, added more Doxygen.


================
Comment at: lib/IR/Globals.cpp:188
+  setGlobalValueSubClassData((~Mask & getGlobalValueSubClassData()) |
+                             (HasSection ? Mask : 0u));
 }
----------------
mehdi_amini wrote:
> I'd write it along these lines:
> 
> ```
> unsigned Mask = 1 << HasSectionHashEntryBit;
> auto Bitfield = getGlobalValueSubClassData();
> if (HasSection) 
>   // Set the `HasSectionHashEntryBit`
>   Bitfield |= Mask;
> else
>   // Clear the `HasSectionHashEntryBit`
>   Bitfield &= ~Mask;
> setGlobalValueSubClassData(Bitfield);
> ```
> 
> Seems less complex to read to me than the `|` with a ternary on the RHS that may lead to a 0.
> 
> Otherwise extracting it in a `setGlobalValueSubClassDataBitValue(unsigned BitNo, bool Value)` would make the implementation of it trivial.
Yeah, setGlobalValueSubClassDataBitValue sounds like the right thing.


https://reviews.llvm.org/D28150