[PATCH] CodeGen: Stick constant pool entries in COMDAT sections for WinCOFF

Jim Grosbach grosbach at apple.com
Mon Jul 14 11:06:27 PDT 2014


No feedback on the Windows specific aspects, as I don't know how that stuff works well enough to comment.

Some general comments inline. A couple of cleanup changes that should be split out into separate patches and an API cleanup to get better layering.

================
Comment at: include/llvm/ADT/StringExtras.h:56
@@ -55,3 +55,3 @@
 template<typename IntTy>
-static inline char *utohex_buffer(IntTy X, char *BufferEnd) {
+static inline char *utohex_buffer(IntTy X, char *BufferEnd, bool LowerCase = false) {
   char *BufPtr = BufferEnd;
----------------
These changes are distinct and should be a separate patch.

Also, documentation comment needs updated to match.

================
Comment at: include/llvm/CodeGen/MachineConstantPool.h:124
@@ -122,1 +123,3 @@
+
+  SectionKind getSectionKind(const TargetMachine &TM) const;
 };
----------------
Moving this to a helper function makes sense, but should be a separate patch.

It should take a datalayout argument, too, not the target machine.

================
Comment at: include/llvm/CodeGen/TargetLoweringObjectFileImpl.h:47
@@ -48,1 +46,3 @@
+  const MCSection *getSectionForConstant(SectionKind Kind, const Constant *C,
+                                         MCStreamer &Streamer) const override;
 
----------------
Having these take an MCStreamer reference is really awkward. If that's necessary, it implies something isn't right about the layering elsewhere.

================
Comment at: lib/Target/X86/X86TargetObjectFile.cpp:166
@@ +165,3 @@
+        if (Sym->isUndefined())
+          Streamer.EmitSymbolAttribute(Sym, MCSA_Global);
+        return S;
----------------
OK, here's the layering issue. These functions should be strictly lookup, but here we're mutating things by calling an output function on the streamer. This function isn't the right place for figuring out the symbol names and emitting them to the streamer. GetCPISymbol() is a better place for that.

http://reviews.llvm.org/D4482






More information about the llvm-commits mailing list