This is an archive of the discontinued LLVM Phabricator instance.

Migrate PGOMemOptSizeOpt to use new OptimizationRemarkEmitter Pass
ClosedPublic

Authored by lenary on Jul 28 2017, 12:59 AM.

Details

Summary

Fixes PR33790.

This patch still needs a yaml-style test, which I shall write tomorrow

Event Timeline

lenary created this revision.Jul 28 2017, 12:59 AM

Two quick notes for feedback, I shall get started on working on a test.

lib/Transforms/Instrumentation/PGOMemOPSizeOpt.cpp
30

This change was added by git clang-format, probably incorrectly.

384

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?

anemet accepted this revision.Jul 29 2017, 9:00 AM

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

lib/Transforms/Instrumentation/PGOMemOPSizeOpt.cpp
30

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.

384

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.

This revision is now accepted and ready to land.Jul 29 2017, 9:00 AM
lenary updated this revision to Diff 108804.Jul 29 2017, 2:13 PM
lenary marked 4 inline comments as done.
  • Address Review Feedback, Add Test
This revision was automatically updated to reflect the committed changes.