Page MenuHomePhabricator

Add llvm.loop.licm.disable metadata
ClosedPublic

Authored by timcorringham on Jul 11 2019, 4:36 AM.

Details

Summary

For some targets the LICM pass can result in sub-optimal code in some
cases where it would be better not to run the pass, but it isn't
always possible to suppress the transformations heuristically.

Where the front-end has insight into such cases it is beneficial
to attach loop metadata to disable the pass - this change adds the
llvm.loop.licm.disable metadata to enable that.

Diff Detail

Event Timeline

timcorringham created this revision.Jul 11 2019, 4:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2019, 4:36 AM
fhahn added a subscriber: fhahn.Jul 11 2019, 4:40 AM

For some targets the LICM pass can result in sub-optimal code in some
cases where it would be better not to run the pass, but it isn't
always possible to suppress the transformations heuristically.

Can you provide more details on what targets those are and what the problems there are? How would the frontend decide? If it is target-specific, a TargetTransformInfo hook might be more suitable.

timcorringham added reviewers: Restricted Project, arsenm.Jul 11 2019, 4:40 AM

The target in this case is AMDGPU, and the problem is that transformations can create excessive register pressure ( but I don't believe this is necessarily unique to that target). The front-end has additional information available which can sometimes be used to identify such cases.

This change will only affect code when the metadata is used, so shouldn't have any adverse effect.

hliao added a subscriber: hliao.Jul 11 2019, 5:58 AM

please update the doc on transformation metadata

Update LangRef doc

Added the new metadata to LangRef.rst

Updating D64557: Add llvm.loop.licm.disable metadata

Can you post a real example? I would expect LICM to be used as a canonicalization pass, and then operations would be sunk as appropriate

Unfortunately I don't believe I have an example that is suitable for publishing. The problem is basically that the code motion is generally a good thing, but it can increase live ranges significantly pushing register pressure up such that performance is degraded, but that impact isn't apparent at the point at which the transformation is performed. Disabling the pass for certain loops is a low-impact pragmatic (but not ideal) solution.

Unfortunately I don't believe I have an example that is suitable for publishing. The problem is basically that the code motion is generally a good thing, but it can increase live ranges significantly pushing register pressure up such that performance is degraded, but that impact isn't apparent at the point at which the transformation is performed. Disabling the pass for certain loops is a low-impact pragmatic (but not ideal) solution.

Is this a rematerialization problem? We do hoist things sometimes which turn into multiple instructions and then we can't rematerialize during RA.

Meinersbur added inline comments.Jul 11 2019, 9:20 AM
lib/Transforms/Scalar/LICM.cpp
335

There is a hasDisableAllTransformsHint (llvm.loop.disable_nonforced) metadata that was intended to disable all (high-level) loop transformations. The use case is to ensure that no pass changes the loop before a user-defined transformation is applied (e.g. a loop is not unrolled before #pragma clang loop interleave(enable) is applied).

The prefix llvm.loop. indicates that LICM is also such a loop transformation and therefore disabled by llvm.loop.disable_nonforced, but I don't think we want that.

Maybe use llvm.licm.disable?

Also, there are several schemes of enable/disable metadata. The other one to disable a transformations is e.g. !{"llvm.loop.vectorize.enable", i1 0}. I am not sure which one is preferable; we have the mix already anyway.

Changed the metadata name to llvm.licm.disable

timcorringham marked 2 inline comments as done.Jul 22 2019, 3:07 AM

Some cases can be undone by rematerialization, but not all, and it can involve a lot of effort which increases compile time. The metadata is a pragmatic approach which helps in some cases.

lib/Transforms/Scalar/LICM.cpp
335

I have changed the metadata name to llvm.licm.disable.

As you note there is a variety of schemes, and I can't see a reason to choose one over another, so I'll leave it as it is.

@fhahn @arsenm @hfinkel Has the motivation question been sufficiently addressed?

test/Transforms/LICM/pragma-licm-disable.ll
5

By convention, the RUN lines are the first in the file, even before comments.

29–37

Could you simplify the test? Remove branching within the loop, at most one PHI, one load (and maybe a store so the load result is not unused).

timcorringham marked an inline comment as done.

Simplified the LIT test

timcorringham marked an inline comment as done.Jul 30 2019, 6:32 AM

Does anyone have any remaining concerns with this as it is now?

Meinersbur accepted this revision.Aug 5 2019, 9:26 AM

Please wait a day before committing in case there are still questions about the motivation.

This revision is now accepted and ready to land.Aug 5 2019, 9:26 AM
fhahn added a comment.Aug 6 2019, 9:09 AM

IMO it's fine to have this solution to address your immediate needs. I still think it would be good if we could get a reproducer, so we can address this properly in LICM.

This revision was automatically updated to reflect the committed changes.