[PATCH] D148328: [IRGen] Change annotation metadata to support a tuple of strings.

[Jessica Paquette via Phabricator via llvm-commits]

paquette added a comment.

> Currently annotation metadata can only support a tuple of strings.

I think the commit message is incorrect?

> For example in remarks any pass that implements annotation remarks can have different type of remarks and pass additional information for each.

Is it possible to test the new tuple behaviour using any existing remarks? Or will that be in a follow-up?



================
Comment at: llvm/lib/CodeGen/MachineFunction.cpp:122
   if (auto &N = Existing->getOperand(0)) {
-    if (cast<MDString>(N.get())->getString() == MetadataName) {
+    if (isa<MDString>(N.get()) &&
+        cast<MDString>(N.get())->getString() == MetadataName) {
----------------
This pattern appears a lot; should it be pulled out into a helper function?


================
Comment at: llvm/lib/IR/Metadata.cpp:1491
+      auto *MDAnnotationTuple = cast<MDTuple>(N);
+      if (any_of(MDAnnotationTuple->operands(), [AnnotationsSet](auto &Op) {
+            return AnnotationsSet.contains(cast<MDString>(Op)->getString());
----------------
Should this be captured by reference instead of copied?


================
Comment at: llvm/lib/Transforms/Scalar/AnnotationRemarks.cpp:67
+        AnnotationStr =
+            cast<MDString>(AnnotationTuple->getOperand(0).get())->getString();
+      }
----------------
Since `AnnotationTuple` is only used in `AnnotationStr`, we can pull it into the `getString()` call and eliminate the braces around the `else`.

Or you could write it via the ternary operator:

```
StringRef AnnotationStr = isa<MDString>(Op.get())) ?
                                             cast<MDString>(Op.get())->getString() :
                                             cast<MDString>(cast<MDTuple>(Op.get())->getOperand(0).get())->getString();
``` 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148328