[llvm-commits] [llvm] r150507 - in /llvm/trunk: include/llvm/CodeGen/TargetLoweringObjectFileImpl.h include/llvm/Target/TargetLoweringObjectFile.h lib/CodeGen/AsmPrinter/AsmPrinter.cpp lib/CodeGen/TargetLoweringObjectFileImpl.cpp

[Chris Lattner]

On Feb 14, 2012, at 1:28 PM, Bill Wendling wrote:

> Author: void
> Date: Tue Feb 14 15:28:13 2012
> New Revision: 150507
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=150507&view=rev
> Log:
> Add code to the target lowering object file module to handle module flags.
> 
> The MachO back-end needs to emit the garbage collection flags specified in the
> module flags. This is a WIP, so the front-end hasn't been modified to emit these
> flags just yet. Documentation and front-end switching to occur soon.

Looks nice, some comments:

+  // Emit module flags.
+  if (NamedMDNode *ModFlags = M.getModuleFlagsMetadata())
+    getObjFileLowering().emitModuleFlags(OutStreamer, ModFlags, Mang, TM);
...

+  /// emitModuleFlags - Emit the module flags that specify the garbage
+  /// collection information.
+  virtual void emitModuleFlags(MCStreamer &Streamer, NamedMDNode *ModFlags,
+                               Mangler *Mang, const TargetMachine &TM) const;

Instead of passing the NamedMDNode down and having all clients have to tear it apart, it would be better for a Module:: api to decompose them into structs and then pass an ArrayRef of structs into TLOF.  Please add a new local struct to Module that contains "ModAttrBehavior Behavior, StringRef Key, Value *Val", and add method to Module that returns the (possibly empty) list for the module.

That would make the AsmPrinter.cpp common code look like:

SmallVector<Module::ModuleFlagEntry, 8> ModuleFlags;
M.getModuleFlags(ModuleFlags);
if (!ModuleFlags.empty()) 
  ... emitModuleFlags(... ModuleFlags, ...)


> +void TargetLoweringObjectFileMachO::
> +emitModuleFlags(MCStreamer &Streamer, NamedMDNode *ModFlags,
> +                Mangler *Mang, const TargetMachine &TM) const {
> +  StringRef Version("Objective-C Image Info Version");
> +  StringRef SectionSpec("Objective-C Image Info Section");
> +  StringRef GC("Objective-C Garbage Collection");
> +  StringRef GCOnly("Objective-C GC Only");

Is there a specific reason you factored these out like this?  Why not just do 

if (KeyStr == "Objective-C Image Info Version")  down below?

> +  // The section is mandatory. If we don't have it, then we don't have image
> +  // info information.
> +  if (Section.empty()) return;
> +
> +  uint32_t Values[2] = { VersionVal, GCFlags };
> +  Module *M = ModFlags->getParent();
> +  Constant *Init = ConstantDataArray::get(M->getContext(), Values);
> +  GlobalVariable *GV = new GlobalVariable(*M, Init->getType(), false,
> +                                          GlobalValue::InternalLinkage, Init,
> +                                          "\01L_OBJC_IMAGE_INFO");
> +  GV->setSection(Section);
> +
> +  MCSymbol *GVSym = Mang->getSymbol(GV);
> +  SectionKind GVKind = TargetLoweringObjectFile::getKindForGlobal(GV, TM);
> +  const MCSection *TheSection = SectionForGlobal(GV, GVKind, Mang, TM);

This should not be creating (and leaking :) a temporary GlobalVariable.  Just drive the Streamer API directly to get the directives you need.

-Chris