Page MenuHomePhabricator

[LoopVectorize] Don't create unnecessary vscale intrinsic calls
ClosedPublic

Authored by david-arm on Apr 19 2021, 7:21 AM.

Details

Summary

In quite a few cases in LoopVectorize.cpp we call createStepForVF
with a step value of 0, which leads to unnecessary generation of
llvm.vscale intrinsic calls. I've optimised IRBuilder::CreateVScale
and createStepForVF to return 0 when attempting to multiply
vscale by 0.

Diff Detail

Event Timeline

david-arm created this revision.Apr 19 2021, 7:21 AM
david-arm requested review of this revision.Apr 19 2021, 7:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2021, 7:21 AM
frasercrmck added inline comments.Apr 20 2021, 1:22 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1119

Not that this is wrong, but would it be inappropriate to make CreateVScale detect a zero step and return zero?

frasercrmck added inline comments.Apr 20 2021, 1:23 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1119

Sorry I just realised that's what you've done. Do we need to make this change, then?

david-arm added inline comments.Apr 20 2021, 1:30 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1119

It's not really necessary as the end result is the same - I was just trying to avoid the compiler doing more work for the Step=0 case, i.e. the creation of a ConstantInt, etc. I noticed there are a few places in the vectoriser where we call this function with a Step of 0 - it happens when widening a PHI instruction or vectorising induction variables.

If you prefer I can just revert this code?

frasercrmck added inline comments.Apr 20 2021, 1:33 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1119

I don't feel particularly strongly. I suppose my initial comment was trying to avoid repeating ourselves and duplicating logic. But I also realise that this is how compile-time regressions creep in. I don't know how impactful the creation of a zero ConstantInt is? Presumably it's cached more often than not?

sdesmalen added inline comments.Apr 20 2021, 1:34 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1119

The overhead of creating the ConstantInt is small, and this only happens on a small number of cases when the step value is actually zero. Personally I'd prefer this to be implemented only once in IRBuilder, and have this change removed to make the code simpler.

david-arm updated this revision to Diff 338804.Apr 20 2021, 3:53 AM
  • Reverted change to createStepForVF
david-arm marked 3 inline comments as done.Apr 20 2021, 3:54 AM

Is it worth adding a test to llvm/unittests/IR/IRBuilderTest.cpp for this change?

david-arm updated this revision to Diff 339154.Apr 21 2021, 2:44 AM
  • Added unit test for vscale * 0 case.
sdesmalen accepted this revision.Apr 21 2021, 2:46 AM

LGTM, thanks for adding the test.

This revision is now accepted and ready to land.Apr 21 2021, 2:46 AM
frasercrmck accepted this revision.Apr 21 2021, 2:46 AM

LGTM too, thanks.