[PATCH] D92887: [LoopUnroll] Use llvm.experimental.noalias.scope.decl for duplicating noalias metadata as needed

[Jeroen Dobbelaere via Phabricator via llvm-commits]

jeroen.dobbelaere added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/Cloning.h:300
+    ArrayRef<BasicBlock *> BBs,
+    SmallVectorImpl<MetadataAsValue *> &out_NoAliasDeclScopes);
+
----------------
nikic wrote:
> I don't think we use this kind of `out_` style anywhere.
I personally like the the out_/inout_/in_ style for making the purpose clear. But I understand that in the LLVM context it is better to have a common style.



================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:694
+      cloneAndAdaptNoAliasScopes(LoopLocalNoAliasDeclScopes, NewBlocks,
+                                 Header->getContext(), ext);
+    }
----------------
nikic wrote:
> Possible the `ext` parameter should be `Twine` rather than `StringRef`. But I'm not really familiar with our string API conventions.
I would rather keep it. When using a Twine, the Twine will be evaluated every time a string is needed.
By using a StringRef, at least that part will only be done once.



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

https://reviews.llvm.org/D92887