r212263 - Move the calling of emitTargetMD() later.

Robert Lytton robert at xmos.com
Thu Jul 17 01:47:49 PDT 2014


Hi Richard,

Thank you for the review.

I'll change the metadata numbers to use filecheck variables.

In regard to using getMostRecentDecl, is the problem that I am using it too soon or the way I use it?
Indeed, if I was to add a pass, would it be needed at all - maybe the pass should handle multiple list entries of a symbol.

I agree, building a target specific list and adding a pass seems good.
Should I handle all global declarations in this way or only those I can't compute?
I need to understand the best places to discover/add-to-list the global declarations.
getTargetCodeGenInfo()::SetTargetAttributes only receives definitions (and their typestrings could be emitted).
Is there another target hook which receives all declarations rather than only definitions?
I'll look through the code again regarding how 'MangledDeclNames' is built up to try to gain insight.

Any guidance most welcome!

Robert

________________________________
From: metafoo at gmail.com [metafoo at gmail.com] on behalf of Richard Smith [richard at metafoo.co.uk]
Sent: 16 July 2014 04:07
To: Robert Lytton
Cc: cfe commits
Subject: Re: r212263 - Move the calling of emitTargetMD() later.

On Thu, Jul 3, 2014 at 2:30 AM, Robert Lytton <robert at xmos.com<mailto:robert at xmos.com>> wrote:
Author: rlytton
Date: Thu Jul  3 04:30:33 2014
New Revision: 212263

URL: http://llvm.org/viewvc/llvm-project?rev=212263&view=rev
Log:
Move the calling of emitTargetMD() later.

Summary:
Because a global created by GetOrCreateLLVMGlobal() is not finalised until later viz:
  extern char a[];
  char f(){ return a[5];}
  char a[10];

Change MangledDeclNames to use a MapVector rather than a DenseMap so that the
Metadata is output in order of original declaration, so to make deterministic
and improve human readablity.

Sorry for not reviewing this before. I think this is completely the wrong approach. Please just keep a list of those variables for which you couldn't compute the type string when they are emitted (within XCoreTargetCodeGenInfo), and perform another pass at the end of IR generation to fix up those ones. That will have much better locality, and will avoid the target metadata emission loop that is redundant on almost all targets.

Differential Revision: http://reviews.llvm.org/D4176

Modified:
    cfe/trunk/lib/CodeGen/CodeGenModule.cpp
    cfe/trunk/lib/CodeGen/CodeGenModule.h
    cfe/trunk/test/CodeGen/xcore-stringtype.c

Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=212263&r1=212262&r2=212263&view=diff
==============================================================================
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Thu Jul  3 04:30:33 2014
@@ -393,6 +393,8 @@ void CodeGenModule::Release() {
     DebugInfo->finalize();

   EmitVersionIdentMetadata();
+
+  EmitTargetMetadata();
 }

 void CodeGenModule::UpdateCompletedType(const TagDecl *TD) {
@@ -1534,8 +1536,6 @@ CodeGenModule::GetOrCreateLLVMFunction(S
     }
   }

-  getTargetCodeGenInfo().emitTargetMD(D, F, *this);
-
   // Make sure the result is of the requested type.
   if (!IsIncompleteFunction) {
     assert(F->getType()->getElementType() == Ty);
@@ -1685,8 +1685,6 @@ CodeGenModule::GetOrCreateLLVMGlobal(Str
   if (AddrSpace != Ty->getAddressSpace())
     return llvm::ConstantExpr::getAddrSpaceCast(GV, Ty);

-  getTargetCodeGenInfo().emitTargetMD(D, GV, *this);
-
   return GV;
 }

@@ -3340,6 +3338,14 @@ void CodeGenModule::EmitVersionIdentMeta
   IdentMetadata->addOperand(llvm::MDNode::get(Ctx, IdentNode));
 }

+void CodeGenModule::EmitTargetMetadata() {
+  for (auto &I : MangledDeclNames) {
+    const Decl *D = I.first.getDecl()->getMostRecentDecl();

This has introduced an iterator invalidation bug that breaks most modules builds: getMostRecentDecl can result in lazy deserialization, and thus in MangledDeclNames having entries added to it.

+    llvm::GlobalValue *GV = GetGlobalValue(I.second);
+    getTargetCodeGenInfo().emitTargetMD(D, GV, *this);
+  }
+}
+
 void CodeGenModule::EmitCoverageFile() {
   if (!getCodeGenOpts().CoverageFile.empty()) {
     if (llvm::NamedMDNode *CUNode = TheModule.getNamedMetadata("llvm.dbg.cu<http://llvm.dbg.cu>")) {

Modified: cfe/trunk/lib/CodeGen/CodeGenModule.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.h?rev=212263&r1=212262&r2=212263&view=diff
==============================================================================
--- cfe/trunk/lib/CodeGen/CodeGenModule.h (original)
+++ cfe/trunk/lib/CodeGen/CodeGenModule.h Thu Jul  3 04:30:33 2014
@@ -349,8 +349,8 @@ class CodeGenModule : public CodeGenType
   /// emitted when the translation unit is complete.
   CtorList GlobalDtors;

-  /// A map of canonical GlobalDecls to their mangled names.
-  llvm::DenseMap<GlobalDecl, StringRef> MangledDeclNames;
+  /// An ordered map of canonical GlobalDecls to their mangled names.
+  llvm::MapVector<GlobalDecl, StringRef> MangledDeclNames;
   llvm::StringMap<GlobalDecl, llvm::BumpPtrAllocator> Manglings;

   /// Global annotations.
@@ -1146,6 +1146,9 @@ private:
   /// \brief Emit the Clang version as llvm.ident metadata.
   void EmitVersionIdentMetadata();

+  /// Emits target specific Metadata for global declarations.
+  void EmitTargetMetadata();
+
   /// Emit the llvm.gcov metadata used to tell LLVM where to emit the .gcno and
   /// .gcda files in a way that persists in .bc files.
   void EmitCoverageFile();

Modified: cfe/trunk/test/CodeGen/xcore-stringtype.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/xcore-stringtype.c?rev=212263&r1=212262&r2=212263&view=diff
==============================================================================
--- cfe/trunk/test/CodeGen/xcore-stringtype.c (original)
+++ cfe/trunk/test/CodeGen/xcore-stringtype.c Thu Jul  3 04:30:33 2014
@@ -9,13 +9,13 @@
 // Please see 'Tools Development Guide' section 2.16.2 for format details:
 // <https://www.xmos.com/download/public/Tools-Development-Guide%28X9114A%29.pdf>

-// CHECK: !xcore.typestrings = !{!0, !1, !2, !3, !4, !5, !6, !7, !8, !9, !10,
-// CHECK: !11, !12, !13, !14, !15, !16, !17, !18, !19, !20, !21, !22, !23,
-// CHECK: !24, !25, !26, !27, !28, !29, !30, !31, !32, !33, !34}
+// CHECK: !xcore.typestrings = !{!1, !2, !3, !4, !5, !6, !7, !8, !9, !10, !11,
+// CHECK: !12, !13, !14, !15, !16, !17, !18, !19, !20, !21, !22, !23, !24, !25,
+// CHECK: !26, !27, !28, !29, !30, !31, !32, !33, !34, !35, !36, !37, !38}

Please fix this test to not hardcode metadata numbers; these are extremely fragile, and will break if we start emitting more metadata for any reason. Instead, use FileCheck variables (http://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-variables).

 // test BuiltinType
-// CHECK: !0 = metadata !{void (i1, i8, i8, i8, i16, i16, i16, i32, i32, i32,
+// CHECK: !1 = metadata !{void (i1, i8, i8, i8, i16, i16, i16, i32, i32, i32,
 // CHECK:      i32, i32, i32, i64, i64, i64, float, double, double)*
 // CHECK:      @builtinType, metadata !"f{0}(b,uc,uc,sc,ss,us,ss,si,ui,si,sl,
 // CHECK:      ul,sl,sll,ull,sll,ft,d,ld)"}
@@ -28,14 +28,14 @@ double _Complex Complex; // not supporte


 // test FunctionType & Qualifiers
-// CHECK: !1 = metadata !{void ()* @gI, metadata !"f{0}()"}
-// CHECK: !2 = metadata !{void (...)* @eI, metadata !"f{0}()"}
-// CHECK: !3 = metadata !{void ()* @gV, metadata !"f{0}(0)"}
-// CHECK: !4 = metadata !{void ()* @eV, metadata !"f{0}(0)"}
-// CHECK: !5 = metadata !{void (i32, ...)* @gVA, metadata !"f{0}(si,va)"}
-// CHECK: !6 = metadata !{void (i32, ...)* @eVA, metadata !"f{0}(si,va)"}
-// CHECK: !7 = metadata !{i32* (i32*)* @gQ, metadata !"f{crv:p(cv:si)}(p(cv:si))"}
-// CHECK: !8 = metadata !{i32* (i32*)* @eQ, metadata !"f{crv:p(cv:si)}(p(cv:si))"}
+// CHECK: !2 = metadata !{void ()* @gI, metadata !"f{0}()"}
+// CHECK: !3 = metadata !{void (...)* @eI, metadata !"f{0}()"}
+// CHECK: !4 = metadata !{void ()* @gV, metadata !"f{0}(0)"}
+// CHECK: !5 = metadata !{void ()* @eV, metadata !"f{0}(0)"}
+// CHECK: !6 = metadata !{void (i32, ...)* @gVA, metadata !"f{0}(si,va)"}
+// CHECK: !7 = metadata !{void (i32, ...)* @eVA, metadata !"f{0}(si,va)"}
+// CHECK: !8 = metadata !{i32* (i32*)* @gQ, metadata !"f{crv:p(cv:si)}(p(cv:si))"}
+// CHECK: !9 = metadata !{i32* (i32*)* @eQ, metadata !"f{crv:p(cv:si)}(p(cv:si))"}
 extern void eI();
 void gI() {eI();};
 extern void eV(void);
@@ -49,10 +49,10 @@ const volatile int* volatile restrict co


 // test PointerType
-// CHECK: !9 = metadata !{i32* (i32*, i32* (i32*)*)*
-// CHECK:      @pointerType, metadata !"f{p(si)}(p(si),p(f{p(si)}(p(si))))"}
-// CHECK: !10 = metadata !{i32** @EP, metadata !"p(si)"}
-// CHECK: !11 = metadata !{i32** @GP, metadata !"p(si)"}
+// CHECK: !10 = metadata !{i32* (i32*, i32* (i32*)*)*
+// CHECK:       @pointerType, metadata !"f{p(si)}(p(si),p(f{p(si)}(p(si))))"}
+// CHECK: !11 = metadata !{i32** @EP, metadata !"p(si)"}
+// CHECK: !12 = metadata !{i32** @GP, metadata !"p(si)"}
 extern int* EP;
 int* GP;
 int* pointerType(int *I, int * (*FP)(int *)) {
@@ -60,25 +60,25 @@ int* pointerType(int *I, int * (*FP)(int
 }

 // test ArrayType
-// CHECK: !12 = metadata !{[2 x i32]* (i32*, i32*, [2 x i32]*, [2 x i32]*, i32*)*
+// CHECK: !13 = metadata !{[2 x i32]* (i32*, i32*, [2 x i32]*, [2 x i32]*, i32*)*
 // CHECK:       @arrayType, metadata !"f{p(a(2:si))}(p(si),p(cv:si),p(a(2:si)),
 // CHECK:       p(a(2:si)),p(si))"}
-// CHECK: !13 = metadata !{[0 x i32]* @EA1, metadata !"a(*:cv:si)"}
-// CHECK: !14 = metadata !{[2 x i32]* @EA2, metadata !"a(2:si)"}
-// CHECK: !15 = metadata !{[0 x [2 x i32]]* @EA3, metadata !"a(*:a(2:si))"}
-// CHECK: !16 = metadata !{[3 x [2 x i32]]* @EA4, metadata !"a(3:a(2:si))"}
-// CHECK: !17 = metadata !{[2 x i32]* @GA1, metadata !"a(2:cv:si)"}
-// CHECK: !18 = metadata !{void ([2 x i32]*)* @arrayTypeVariable1,
+// CHECK: !14 = metadata !{[0 x i32]* @EA1, metadata !"a(*:cv:si)"}
+// CHECK: !15 = metadata !{[2 x i32]* @EA2, metadata !"a(2:si)"}
+// CHECK: !16 = metadata !{[0 x [2 x i32]]* @EA3, metadata !"a(*:a(2:si))"}
+// CHECK: !17 = metadata !{[3 x [2 x i32]]* @EA4, metadata !"a(3:a(2:si))"}
+// CHECK: !18 = metadata !{[2 x i32]* @GA1, metadata !"a(2:cv:si)"}
+// CHECK: !19 = metadata !{void ([2 x i32]*)* @arrayTypeVariable1,
 // CHECK:       metadata !"f{0}(p(a(2:si)))"}
-// CHECK: !19 = metadata !{void (void ([2 x i32]*)*)* @arrayTypeVariable2,
+// CHECK: !20 = metadata !{void (void ([2 x i32]*)*)* @arrayTypeVariable2,
 // CHECK:       metadata !"f{0}(p(f{0}(p(a(2:si)))))"}
-// CHECK: !20 = metadata !{[3 x [2 x i32]]* @GA2, metadata !"a(3:a(2:si))"}
+// CHECK: !21 = metadata !{[3 x [2 x i32]]* @GA2, metadata !"a(3:a(2:si))"}
+extern int GA2[3][2];
 extern const volatile int EA1[];
 extern int EA2[2];
 extern int EA3[][2];
 extern int EA4[3][2];
 const volatile int GA1[2];
-int GA2[3][2];
 extern void arrayTypeVariable1(int[*][2]);
 extern void arrayTypeVariable2( void(*fp)(int[*][2]) );
 extern void arrayTypeVariable3(int[3][*]);                // not supported
@@ -100,16 +100,16 @@ RetType* arrayType(int A1[], int const v


 // test StructureType
-// CHECK: !21 = metadata !{void (%struct.S1*)* @structureType1, metadata
+// CHECK: !22 = metadata !{void (%struct.S1*)* @structureType1, metadata
 // CHECK:       !"f{0}(s(S1){m(ps2){p(s(S2){m(ps3){p(s(S3){m(s1){s(S1){}}})}})}})"}
-// CHECK: !22 = metadata !{void (%struct.S2*)* @structureType2, metadata
+// CHECK: !23 = metadata !{void (%struct.S2*)* @structureType2, metadata
 // CHECK:       !"f{0}(s(S2){m(ps3){p(s(S3){m(s1){s(S1){m(ps2){p(s(S2){})}}}})}})"}
-// CHECK: !23 = metadata !{void (%struct.S3*)* @structureType3, metadata
+// CHECK: !24 = metadata !{void (%struct.S3*)* @structureType3, metadata
 // CHECK:       !"f{0}(s(S3){m(s1){s(S1){m(ps2){p(s(S2){m(ps3){p(s(S3){})}})}}}})"}
-// CHECK: !24 = metadata !{void (%struct.S4*)* @structureType4, metadata
+// CHECK: !25 = metadata !{void (%struct.S4*)* @structureType4, metadata
 // CHECK:       !"f{0}(s(S4){m(s1){s(S1){m(ps2){p(s(S2){m(ps3){p(s(S3){m(s1){s(S1){}}})}})}}}})"}
-// CHECK: !25 = metadata !{%struct.anon* @StructAnon, metadata !"s(){m(A){si}}"}
-// CHECK: !26 = metadata !{i32 (%struct.SB*)* @structureTypeB, metadata
+// CHECK: !26 = metadata !{%struct.anon* @StructAnon, metadata !"s(){m(A){si}}"}
+// CHECK: !27 = metadata !{i32 (%struct.SB*)* @structureTypeB, metadata
 // CHECK:       !"f{si}(s(SB){m(){b(4:si)},m(){b(2:si)},m(N4){b(4:si)},
 // CHECK:       m(N2){b(2:si)},m(){b(4:ui)},m(){b(4:si)},m(){b(4:c:si)},
 // CHECK:       m(){b(4:c:si)},m(){b(4:cv:si)}})"}
@@ -130,16 +130,16 @@ int structureTypeB(struct SB sb){return


 // test UnionType
-// CHECK: !27 = metadata !{void (%union.U1*)* @unionType1, metadata
+// CHECK: !28 = metadata !{void (%union.U1*)* @unionType1, metadata
 // CHECK:       !"f{0}(u(U1){m(pu2){p(u(U2){m(pu3){p(u(U3){m(u1){u(U1){}}})}})}})"}
-// CHECK: !28 = metadata !{void (%union.U2*)* @unionType2, metadata
+// CHECK: !29 = metadata !{void (%union.U2*)* @unionType2, metadata
 // CHECK:       !"f{0}(u(U2){m(pu3){p(u(U3){m(u1){u(U1){m(pu2){p(u(U2){})}}}})}})"}
-// CHECK: !29 = metadata !{void (%union.U3*)* @unionType3, metadata
+// CHECK: !30 = metadata !{void (%union.U3*)* @unionType3, metadata
 // CHECK:       !"f{0}(u(U3){m(u1){u(U1){m(pu2){p(u(U2){m(pu3){p(u(U3){})}})}}}})"}
-// CHECK: !30 = metadata !{void (%union.U4*)* @unionType4, metadata
+// CHECK: !31 = metadata !{void (%union.U4*)* @unionType4, metadata
 // CHECK:       !"f{0}(u(U4){m(u1){u(U1){m(pu2){p(u(U2){m(pu3){p(u(U3){m(u1){u(U1){}}})}})}}}})"}
-// CHECK: !31 = metadata !{%union.anon* @UnionAnon, metadata !"u(){m(A){si}}"}
-// CHECK: !32 = metadata !{i32 (%union.UB*)* @unionTypeB, metadata
+// CHECK: !32 = metadata !{%union.anon* @UnionAnon, metadata !"u(){m(A){si}}"}
+// CHECK: !33 = metadata !{i32 (%union.UB*)* @unionTypeB, metadata
 // CHECK:       !"f{si}(u(UB){m(N2){b(2:si)},m(N4){b(4:si)},m(){b(2:si)},
 // CHECK:       m(){b(4:c:si)},m(){b(4:c:si)},m(){b(4:cv:si)},m(){b(4:si)},
 // CHECK:       m(){b(4:si)},m(){b(4:ui)}})"}
@@ -160,9 +160,20 @@ int unionTypeB(union UB ub) {return Unio


 // test EnumType
-// CHECK: !33 = metadata !{i32* @EnumAnon, metadata !"e(){m(EA){3}}"}
-// CHECK: !34 = metadata !{i32 (i32)* @enumType, metadata
+// CHECK: !34 = metadata !{i32* @EnumAnon, metadata !"e(){m(EA){3}}"}
+// CHECK: !35 = metadata !{i32 (i32)* @enumType, metadata
 // CHECK:       !"f{si}(e(E){m(A){7},m(B){6},m(C){5},m(D){0}})"}
 enum E {D, C=5, B, A};
 enum {EA=3} EnumAnon = EA;
 int enumType(enum E e) {return EnumAnon;}
+
+
+// CHECK: !36 = metadata !{i32 ()* @testReDecl, metadata !"f{si}()"}
+// CHECK: !37 = metadata !{[10 x i32]* @After, metadata !"a(10:si)"}
+// CHECK: !38 = metadata !{[10 x i32]* @Before, metadata !"a(10:si)"}
+extern int After[];
+extern int Before[10];
+int testReDecl() {return After[0] + Before[0];}
+int After[10];
+int Before[];
+


_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140717/20f3225d/attachment.html>


More information about the cfe-commits mailing list