This is an archive of the discontinued LLVM Phabricator instance.

Rename getMaximumUnrollFactor().
ClosedPublic

Authored by spatel on Aug 26 2014, 8:28 AM.

Details

Summary

"getMaximumUnrollFactor()" isn't used by the loop unroller at all. It's strictly a loop vectorizer variable, so I propose renaming it to "getMaxVectorUnrollFactor()" with this patch. No functional change intended.

Ideally, I think we would move this subtarget-specific variable into the SchedMachineModel. That's where we already have a variable for "LoopMicroOpBufferSize" which is used by the loop unroller. If that's a good change, let me know, and I'll work on a patch for it.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel updated this revision to Diff 12949.Aug 26 2014, 8:28 AM
spatel retitled this revision from to Rename getMaximumUnrollFactor()..
spatel updated this object.
spatel edited the test plan for this revision. (Show Details)
spatel added reviewers: aschwaighofer, hfinkel, nadav.
spatel added a subscriber: Unknown Object (MLST).
hfinkel edited edge metadata.Aug 26 2014, 8:32 AM

I agree that this should be renamed. We actually call this the interleaving factor (not the unrolling factor) in user-facing things (like the pragmas and metadata); I think the renaming should reflect that.

I agree with Hal, interleave factor is a much better name, and is independent of the loop (or any other) vectorizer. The comments should also be changed to explain that.

--renato

spatel updated this revision to Diff 12954.Aug 26 2014, 10:35 AM
spatel added a reviewer: rengolin.

Thanks very much for the feedback, Hal and Renato.

I wasn't aware of the pragmas in clang - good to know about those.

In this version of the patch, I have changed the name to "getMaxVectorInterleaveFactor". I also made a bunch of interior changes in LoopVectorizer...but there are 2 "seams" where we still use "unroll":

  1. I stopped short of changing the cl::opt names - if we really want to unify on "interleave" I assume we want to change those too so they match the pragmas? That will require changing 80+ test cases in test-suite. If everyone agrees that is better, I'll certainly make that change too.
  1. Eventually the interleave factor becomes an ingredient in "UF", the unroll factor: if (UF > MaxInterleaveSize) UF = MaxInterleaveSize;

I have not looked at the logic in LV close enough to know if "UF" is the appropriate name. Any opinions?

I also thing it makes sense to rename this to interleave factor to point out the difference between interleaved unrolling vs concatenating unrolling. Interleaving is not necessarily tied to vectorization. The fact that we interleave in the vectorizer is just an artifact of our implementation. I think it should be getMaxInterleaveFactor(). We are not necessarily interleaving vector instructions.

Thanks for cleaning this up!

rengolin edited edge metadata.Aug 26 2014, 1:03 PM

Hi Sanjay,

In this version of the patch, I have changed the name to "getMaxVectorInterleaveFactor". I also made a bunch of interior changes in LoopVectorizer...but there are 2 "seams" where we still use "unroll":

Please, drop the "Vector" as it's not directly related to the interleaving.

  1. I stopped short of changing the cl::opt names - if we really want to unify on "interleave" I assume we want to change those too so they match the pragmas? That will require changing 80+ test cases in test-suite. If everyone agrees that is better, I'll certainly make that change too.

That's a good point. We certainly could, but that'd have to be an alias, at least for now.

Arnold, how public are those flags nowadays? The decision to either keep it forever, rename now or anything in between will have to be based on that.

  1. Eventually the interleave factor becomes an ingredient in "UF", the unroll factor: if (UF > MaxInterleaveSize) UF = MaxInterleaveSize;

I have not looked at the logic in LV close enough to know if "UF" is the appropriate name. Any opinions?

I think the name is correct as it is.

IIRC, UF is the actual unrolling factor, which bases its decision ultimately on the interleave factor, since it's generally not profitable to "just unroll" at a factor greater than the interleave, even if the data dependencies allow it.

cheers,
--renato

spatel updated this revision to Diff 12963.Aug 26 2014, 1:24 PM
spatel edited edge metadata.

Changed name to: "getMaxInterleaveFactor" - sorry that I missed the earlier comment that this is independent of the vectorizer.

spatel added a comment.Sep 3 2014, 9:11 AM

Ping.

There's still a question of whether to change the cl::opt names either in this patch or later?

  • Original Message -----

From: "Sanjay Patel" <spatel@rotateright.com>
To: spatel@rotateright.com, nrotem@apple.com, hfinkel@anl.gov, "renato golin" <renato.golin@linaro.org>,
aschwaighofer@apple.com
Cc: mcrosier@codeaurora.org, llvm-commits@cs.uiuc.edu
Sent: Wednesday, September 3, 2014 11:11:54 AM
Subject: Re: [PATCH] Rename getMaximumUnrollFactor().

Ping.

There's still a question of whether to change the cl::opt names
either in this patch or later?

Those are for development and debugging. I think that either is fine, but you might as well do it now for consistency.

-Hal

http://reviews.llvm.org/D5066

spatel updated this revision to Diff 13482.Sep 9 2014, 10:58 AM

Rebased and updated to change the cl::opt params - and so affects lots of LV testcases that use that param.

Please let me know if this is ok to commit. Thanks!

rengolin accepted this revision.Sep 10 2014, 3:04 AM
rengolin edited edge metadata.

Hi Sanjay,

LGTM, Thanks!

This revision is now accepted and ready to land.Sep 10 2014, 3:04 AM
spatel closed this revision.Sep 10 2014, 11:08 AM
spatel updated this revision to Diff 13553.

Closed by commit rL217528 (authored by @spatel).

Thanks all for the review - committed at r217528.