Page MenuHomePhabricator

[LV] Consider Loop Unroll Hints When Making Interleave Decisions
ClosedPublic

Authored by bmahjour on Apr 27 2021, 9:16 AM.

Details

Summary

Currently the #pragma clang loop unroll(disable) directive has no impact on loop vectorizer's decision to interleave or not. This results in unintended codegen when users explicitly ask for no unrolling to take place.
This patch causes the loop vectorizer to not interleave loops that have nounroll loop hints (llvm.loop.unroll.disable and llvm.loop.unroll_count(1)). Note that if a particular interleave count is being requested (through llvm.loop.interleave_count), it will still be honoured, regardless of the presence of nounroll hints.

Diff Detail

Event Timeline

bmahjour created this revision.Apr 27 2021, 9:16 AM
bmahjour requested review of this revision.Apr 27 2021, 9:16 AM
bmahjour edited the summary of this revision. (Show Details)Apr 27 2021, 9:16 AM
bmahjour edited the summary of this revision. (Show Details)

Consider using hasUnrollTransformation() & TM_Disable (LoopUtils.h) instead. I think it is worth implementing the interpretation of when unrolling is disabled at a single place.

llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
49

I like the rename of HK_UNROLL to HK_INTERLEAVE

bmahjour updated this revision to Diff 341031.Apr 27 2021, 5:12 PM
bmahjour edited the summary of this revision. (Show Details)

Changed it to use hasUnrollTransformation() per Michael's comment. Note that hasUnrollTransformation does not take llvm.loop.unroll.runtime.disable into account. I think that's ok for now, since clang doesn't seem to be generating that MD and we only use it for marking epilogue loops. We could add that in the future as a separate patch, if the need arises.

Meinersbur accepted this revision.EditedApr 28 2021, 1:25 PM

LGTM, after some in-source comment about the motivation is added.

Note that hasUnrollTransformation does not take llvm.loop.unroll.runtime.disable into account. I think that's ok for now, since clang doesn't seem to be generating that MD and we only use it for marking epilogue loops. We could add that in the future as a separate patch, if the need arises.

It was based that function on what Clang generates with the primary intention to generate "your pragma has been ignored" warnings. llvm.loop.unroll.runtime.disable would still allow full unrolling, so it does not fit directly into an enable/disable scheme. Maybe we'd need separate hasFullUnrollTransformation and hasPartialUnrollTransformation functions. Patch welcome.

llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
117

Could you add a comment about what the intention of this is?
Suggestion:

// If interleaving is not explicitly set, assume that if we do not want unrolling, we also don't want any interleaving.
This revision is now accepted and ready to land.Apr 28 2021, 1:25 PM
bmahjour marked an inline comment as done.Apr 28 2021, 2:04 PM

It was based that function on what Clang generates with the primary intention to generate "your pragma has been ignored" warnings. llvm.loop.unroll.runtime.disable would still allow full unrolling, so it does not fit directly into an enable/disable scheme. Maybe we'd need separate hasFullUnrollTransformation and hasPartialUnrollTransformation functions. Patch welcome.

I see. We should also update the documentation to be clear about the semantics of llvm.loop.unroll.runtime.disable, I think.

llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
117

done.

This revision was automatically updated to reflect the committed changes.
bmahjour marked an inline comment as done.
weiwang added inline comments.
llvm/test/Transforms/LoopVectorize/nounroll.ll
1

I saw this test failed in my local build and also from build bots: http://lab.llvm.org:8011/#/builders/67/builds/2504 with the following error:

Input was:
<<<<<<
           1: opt: Unknown command line argument '-debug-only=loop-vectorize'. Try: '/home/buildbot/as-worker-92/clang-with-thin-lto-ubuntu/build/stage1/bin/opt --help' 
check:5'0     X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
check:5'1      ?                                                                                                                                                          possible intended match
           2: opt: Did you mean '--debug-pass=loop-vectorize'? 
check:5'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>
--

I think -debug-only is only available when -DLLVM_ENABLE_ASSERTIONS=ON.

SjoerdMeijer added inline comments.Apr 29 2021, 12:36 AM
llvm/test/Transforms/LoopVectorize/nounroll.ll
1

Thanks for reporting, should be fixed with rG837fded984ed.

bmahjour added inline comments.Apr 29 2021, 6:55 AM
llvm/test/Transforms/LoopVectorize/nounroll.ll
1

Ah, sorry I missed that. Thanks for the fix @SjoerdMeijer