[PATCH] D35981: Migrate PGOMemOptSizeOpt to use new OptimizationRemarkEmitter Pass

[Adam Nemet via Phabricator via llvm-commits]

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

LGTM, I suppose there is already a -pass-remark test for this.  Of course it would be great to also have a YAML test.



================
Comment at: lib/Transforms/Instrumentation/PGOMemOPSizeOpt.cpp:30
 #include "llvm/IR/IRBuilder.h"
+#include "llvm/IR/InstVisitor.h"
 #include "llvm/IR/InstrTypes.h"
----------------
lenary wrote:
> This change was added by `git clang-format`, probably incorrectly. 
It probably orders uppercase before lowercase.  This is fine.  I usually land changes like this in a commit separate from the functional change but it's fine either way in this case.


================
Comment at: lib/Transforms/Instrumentation/PGOMemOPSizeOpt.cpp:384
+    using namespace ore;
+    ORE.emit(OptimizationRemark(DEBUG_TYPE, "memopt-opt", MI)
+             << "optimized " << getMIName(MI) << " with count "
----------------
lenary wrote:
> Not sure this is the right pair of names for this remark, maybe I should use `getMIName(MI)` (which returns "memset"/"memcpy"/"memmove"/"unknown") for the second string as well?
No, I think it's fine the way it is.   I think that it's better to have static strings for the remark name/identifier.

One nice improvement would be to pass the intrinsic name as an attribute so that we'd get "Intrinsic: memset" in YAML with NV("Intrinsic", getMIName(MI)).

I don't think we have an override for the Argument ctor that would take a StringRef.  You may need to add that.  This is also fine as a follow-on.  


https://reviews.llvm.org/D35981