This is an archive of the discontinued LLVM Phabricator instance.

[GVN] Basic optimization remark support
ClosedPublic

Authored by anemet on Nov 9 2016, 9:21 PM.

Event Timeline

anemet updated this revision to Diff 77442.Nov 9 2016, 9:21 PM
anemet retitled this revision from to [GVN] Basic optimization remark support.
anemet updated this object.
anemet added reviewers: hfinkel, dberlin.
anemet added a subscriber: llvm-commits.
davide added a subscriber: davide.Nov 9 2016, 11:21 PM

You're mentioning that more interesting cases will be added in later patches, can you please elaborate what else do you have in mind (other than load elim?)

lib/IR/DiagnosticInfo.cpp
183–188

I assume you're using this but it's not strictly tied to the patch, i.e. maybe can be committed separately? (up to you)

lib/Transforms/Scalar/GVN.cpp
1811–1812

I see here you're adding this DEBUG statement before emitting the remark but you don't in the other two cases. Is this intended?

Hi Davide,

You're mentioning that more interesting cases will be added in later patches, can you please elaborate what else do you have in mind (other than load elim?)

Currently it is load-elim in patch 2 and 3. There are probably other interesting things we can do with load-pre but I don't have those implemented. Also not directly GVN-specific, but it would also be nice to somehow expose the aliasing information via opt-viewer. (Let me know if you're interested working on any of this.)

lib/IR/DiagnosticInfo.cpp
183–188

Yeah the first use is in this patch and this allows writing the test case for the feature.

lib/Transforms/Scalar/GVN.cpp
1811–1812

Thanks for bringing this up, I haven't so far thought about the relationship of debug output and opt remarks. I think that we shouldn't have debug output if we already have opt remarks. It makes the code less readable and with "opt -pass-remarks*=" you can get them printed.

Let me remove this.

Perhaps in the long run we can imply -pass-remarks*=<pass> if "-debug-only=<pass>" is used.

anemet updated this revision to Diff 77504.Nov 10 2016, 9:28 AM

Remove the debug statement per Davide's comment

hfinkel edited edge metadata.Nov 11 2016, 5:29 PM

Should we update the message to also provide the location/type of the store? (or the other load for the PRE case).

When we have debug information on the variable, should we print its name/type from the debug info? (This might be useful when we have aggregates, for example, especially after SROA is done with them). I'm happy for this to be done in follow-up if you agree it is useful.

lib/Transforms/Scalar/GVN.cpp
1813

Please make a function to emit this remark; I don't want to repeat literal strings in the code.

Should we update the message to also provide the location/type of the store? (or the other load for the PRE case).

Which store, the one that we forward from? I did that in D26489.

When we have debug information on the variable, should we print its name/type from the debug info? (This might be useful when we have aggregates, for example, especially after SROA is done with them). I'm happy for this to be done in follow-up if you agree it is useful.

Yes I agree, we should explore this further.

anemet updated this revision to Diff 77838.Nov 14 2016, 10:45 AM
anemet edited edge metadata.

Address Hal's comment

hfinkel accepted this revision.Nov 22 2016, 6:12 AM
hfinkel edited edge metadata.

LGTM

This revision is now accepted and ready to land.Nov 22 2016, 6:12 AM
This revision was automatically updated to reflect the committed changes.