[PATCH] D60302: [CodeGen][ObjC] Emit the retainRV marker as a module flag instead of named metadata.

[Akira Hatanaka via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL358048: [CodeGen][ObjC] Emit the retainRV marker as a module 
flag instead of (authored by ahatanak, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D60302?vs=193823&id=194450#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60302

Files:
  cfe/trunk/lib/CodeGen/CGObjC.cpp
  cfe/trunk/test/CodeGenObjC/arc-unsafeclaim.m


Index: cfe/trunk/test/CodeGenObjC/arc-unsafeclaim.m
===
--- cfe/trunk/test/CodeGenObjC/arc-unsafeclaim.m
+++ cfe/trunk/test/CodeGenObjC/arc-unsafeclaim.m
@@ -231,4 +231,5 @@
 
 // This is always at the end of the module.
 
-// CHECK-OPTIMIZED: !clang.arc.retainAutoreleasedReturnValueMarker = !{!0}
+// CHECK-OPTIMIZED: !llvm.module.flags = !{!0,
+// CHECK-OPTIMIZED: !0 = !{i32 1, 
!"clang.arc.retainAutoreleasedReturnValueMarker", !"mov{{.*}}marker for 
objc_retainAutoreleaseReturnValue"}
Index: cfe/trunk/lib/CodeGen/CGObjC.cpp
===
--- cfe/trunk/lib/CodeGen/CGObjC.cpp
+++ cfe/trunk/lib/CodeGen/CGObjC.cpp
@@ -2161,14 +2161,10 @@
 // with this marker yet, so leave a breadcrumb for the ARC
 // optimizer to pick up.
 } else {
-  llvm::NamedMDNode *metadata =
-CGF.CGM.getModule().getOrInsertNamedMetadata(
-"clang.arc.retainAutoreleasedReturnValueMarker");
-  assert(metadata->getNumOperands() <= 1);
-  if (metadata->getNumOperands() == 0) {
-auto &ctx = CGF.getLLVMContext();
-metadata->addOperand(llvm::MDNode::get(ctx,
- llvm::MDString::get(ctx, assembly)));
+  const char *markerKey = "clang.arc.retainAutoreleasedReturnValueMarker";
+  if (!CGF.CGM.getModule().getModuleFlag(markerKey)) {
+auto *str = llvm::MDString::get(CGF.getLLVMContext(), assembly);
+CGF.CGM.getModule().addModuleFlag(llvm::Module::Error, markerKey, str);
   }
 }
   }


Index: cfe/trunk/test/CodeGenObjC/arc-unsafeclaim.m
===
--- cfe/trunk/test/CodeGenObjC/arc-unsafeclaim.m
+++ cfe/trunk/test/CodeGenObjC/arc-unsafeclaim.m
@@ -231,4 +231,5 @@
 
 // This is always at the end of the module.
 
-// CHECK-OPTIMIZED: !clang.arc.retainAutoreleasedReturnValueMarker = !{!0}
+// CHECK-OPTIMIZED: !llvm.module.flags = !{!0,
+// CHECK-OPTIMIZED: !0 = !{i32 1, !"clang.arc.retainAutoreleasedReturnValueMarker", !"mov{{.*}}marker for objc_retainAutoreleaseReturnValue"}
Index: cfe/trunk/lib/CodeGen/CGObjC.cpp
===
--- cfe/trunk/lib/CodeGen/CGObjC.cpp
+++ cfe/trunk/lib/CodeGen/CGObjC.cpp
@@ -2161,14 +2161,10 @@
 // with this marker yet, so leave a breadcrumb for the ARC
 // optimizer to pick up.
 } else {
-  llvm::NamedMDNode *metadata =
-CGF.CGM.getModule().getOrInsertNamedMetadata(
-"clang.arc.retainAutoreleasedReturnValueMarker");
-  assert(metadata->getNumOperands() <= 1);
-  if (metadata->getNumOperands() == 0) {
-auto &ctx = CGF.getLLVMContext();
-metadata->addOperand(llvm::MDNode::get(ctx,
- llvm::MDString::get(ctx, assembly)));
+  const char *markerKey = "clang.arc.retainAutoreleasedReturnValueMarker";
+  if (!CGF.CGM.getModule().getModuleFlag(markerKey)) {
+auto *str = llvm::MDString::get(CGF.getLLVMContext(), assembly);
+CGF.CGM.getModule().addModuleFlag(llvm::Module::Error, markerKey, str);
   }
 }
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60302: [CodeGen][ObjC] Emit the retainRV marker as a module flag instead of named metadata.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D60302#1460379 , @ahatanak wrote:

> Thanks. Does this require changes to swift too?


Yes, it will, to `getObjCRetainAutoreleasedReturnValueMarker` in `GenObjC.cpp`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60302



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60302: [CodeGen][ObjC] Emit the retainRV marker as a module flag instead of named metadata.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

Thanks. Does this require changes to swift too?


Repository:
  rC Clang

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

https://reviews.llvm.org/D60302



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60302: [CodeGen][ObjC] Emit the retainRV marker as a module flag instead of named metadata.

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

Okay.  In that case, I have no objections to this.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60302



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60302: [CodeGen][ObjC] Emit the retainRV marker as a module flag instead of named metadata.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

The upgrader rewrites the named metadata into a module flag (see 
https://reviews.llvm.org/D60303).


Repository:
  rC Clang

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

https://reviews.llvm.org/D60302



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60302: [CodeGen][ObjC] Emit the retainRV marker as a module flag instead of named metadata.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Will the ARC-contract pass recognize both, or are you adding a BC upgrader that 
rewrites the global metadata into a module flag?


Repository:
  rC Clang

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

https://reviews.llvm.org/D60302



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60302: [CodeGen][ObjC] Emit the retainRV marker as a module flag instead of named metadata.

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

I talked with Akira offline and we think this is probably the best approach to 
fix this LTO issue. I will leave others to comment if they think otherwise.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60302



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60302: [CodeGen][ObjC] Emit the retainRV marker as a module flag instead of named metadata.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

Corresponding llvm patch is here: https://reviews.llvm.org/D60303.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60302



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60302: [CodeGen][ObjC] Emit the retainRV marker as a module flag instead of named metadata.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak created this revision.
ahatanak added reviewers: steven_wu, dexonsmith, rjmccall.
ahatanak added a project: clang.
Herald added subscribers: jkorous, kristof.beyls, javed.absar, mehdi_amini.

This fixes a bug which causes the ARC contract pass to not insert the assembly 
marker that is needed for the runtime to do the `objc_autoreleaseReturnValue/ 
objc_retainAutoreleasedReturnValue` optimization when compiling on ARM64/ARM 
with LTO enabled. This happens because `IRLinker::linkNamedMDNodes()` adds all 
the operands from the source modules to the metadata's operand list in the 
merged module (see the example below) and ARC contract fails to extract the 
marker string if the metadata has more than one operand.

  clang.arc.retainAutoreleasedReturnValueMarker = !{!0, !0}
  !0 = !{!"mov\09fp, fp\09\09# marker for objc_retainAutoreleaseReturnValue"}

To fix the bug, this patch emits the marker as a module flag.

rdar://problem/49464214


Repository:
  rC Clang

https://reviews.llvm.org/D60302

Files:
  lib/CodeGen/CGObjC.cpp
  test/CodeGenObjC/arc-unsafeclaim.m


Index: test/CodeGenObjC/arc-unsafeclaim.m
===
--- test/CodeGenObjC/arc-unsafeclaim.m
+++ test/CodeGenObjC/arc-unsafeclaim.m
@@ -231,4 +231,5 @@
 
 // This is always at the end of the module.
 
-// CHECK-OPTIMIZED: !clang.arc.retainAutoreleasedReturnValueMarker = !{!0}
+// CHECK-OPTIMIZED: !llvm.module.flags = !{!0,
+// CHECK-OPTIMIZED: !0 = !{i32 1, 
!"clang.arc.retainAutoreleasedReturnValueMarker", !"mov{{.*}}marker for 
objc_retainAutoreleaseReturnValue"}
Index: lib/CodeGen/CGObjC.cpp
===
--- lib/CodeGen/CGObjC.cpp
+++ lib/CodeGen/CGObjC.cpp
@@ -2161,14 +2161,10 @@
 // with this marker yet, so leave a breadcrumb for the ARC
 // optimizer to pick up.
 } else {
-  llvm::NamedMDNode *metadata =
-CGF.CGM.getModule().getOrInsertNamedMetadata(
-"clang.arc.retainAutoreleasedReturnValueMarker");
-  assert(metadata->getNumOperands() <= 1);
-  if (metadata->getNumOperands() == 0) {
-auto &ctx = CGF.getLLVMContext();
-metadata->addOperand(llvm::MDNode::get(ctx,
- llvm::MDString::get(ctx, assembly)));
+  const char *markerKey = "clang.arc.retainAutoreleasedReturnValueMarker";
+  if (!CGF.CGM.getModule().getModuleFlag(markerKey)) {
+auto *str = llvm::MDString::get(CGF.getLLVMContext(), assembly);
+CGF.CGM.getModule().addModuleFlag(llvm::Module::Error, markerKey, str);
   }
 }
   }


Index: test/CodeGenObjC/arc-unsafeclaim.m
===
--- test/CodeGenObjC/arc-unsafeclaim.m
+++ test/CodeGenObjC/arc-unsafeclaim.m
@@ -231,4 +231,5 @@
 
 // This is always at the end of the module.
 
-// CHECK-OPTIMIZED: !clang.arc.retainAutoreleasedReturnValueMarker = !{!0}
+// CHECK-OPTIMIZED: !llvm.module.flags = !{!0,
+// CHECK-OPTIMIZED: !0 = !{i32 1, !"clang.arc.retainAutoreleasedReturnValueMarker", !"mov{{.*}}marker for objc_retainAutoreleaseReturnValue"}
Index: lib/CodeGen/CGObjC.cpp
===
--- lib/CodeGen/CGObjC.cpp
+++ lib/CodeGen/CGObjC.cpp
@@ -2161,14 +2161,10 @@
 // with this marker yet, so leave a breadcrumb for the ARC
 // optimizer to pick up.
 } else {
-  llvm::NamedMDNode *metadata =
-CGF.CGM.getModule().getOrInsertNamedMetadata(
-"clang.arc.retainAutoreleasedReturnValueMarker");
-  assert(metadata->getNumOperands() <= 1);
-  if (metadata->getNumOperands() == 0) {
-auto &ctx = CGF.getLLVMContext();
-metadata->addOperand(llvm::MDNode::get(ctx,
- llvm::MDString::get(ctx, assembly)));
+  const char *markerKey = "clang.arc.retainAutoreleasedReturnValueMarker";
+  if (!CGF.CGM.getModule().getModuleFlag(markerKey)) {
+auto *str = llvm::MDString::get(CGF.getLLVMContext(), assembly);
+CGF.CGM.getModule().addModuleFlag(llvm::Module::Error, markerKey, str);
   }
 }
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits