LGTM

ping?

LGTM. Good catch!

I agree with Jingyue's cleanup suggestion. Otherwise LGTM.

Test added. Unfortunately the stand alone Eigen3 benchmarks don't show much improvement with this patch because, I believe, they use 32-bit indices throughout. Where we see the huge speedup is in the larger-scale benchmarks using Eigen with 64-bit indices.

Thanks!

Ping, Hal? Thanks!

From your earlier comments it wasn't clear whether you were suggesting that
the front end should add unroll.full and unroll.disable.runtime metadata
with "#pragma unroll" OR if unroll.full metadata should imply no runtime
unrolling in the optimizer. I opted for the latter. It seemed more natural
for the front-end just to do a straight-forward map of the directives to
metadata, and then the optimizer has the logic to properly deal with funny
cases like pragma unroll on a runtime trip count loop.

Removing the llvm-specific flag sounds good to me. Updated in diff.

Update unrolling behavior for unroll(full) directive as discussed in http://reviews.llvm.org/D10857.

LGTM. Just one nit comment/question.

LGTM. Thanks!

Thanks for fixing this. This certainly seems saner than what is there now. Some clarifying comments about MaxSize might be good. Superficially it looks like some global budget because it is a pass level value and gets decremented per loop, but in reality all the budget gets returned when the pass is done with a loop and all of it's clones.

Sorry for the delay. LGTM.

LGTM

LGTM

LGTM

LGTM

LGTM.

LGTM.

LGTM

LGTM with the addition of the new flag to the unit tests.

LGTM.

LGTM.

There was a discussion on the mailing list about the exact issue this patch is addressing. We (Google team working on NVPTX backend) ran into this same issue as did someone (Francois Pichet) working with the ppc backend. Here's the link to the discussion:

LGTM.

LGTM. Thanks!

LGTM. Thanks!

Thanks, Jingyue. LGTM

LGTM

LGTM

Andrew, any comment on this? This just reverts back to a previous more
conservative (and correct) analysis.

Small nit.

Commited r219517.

Just fyi, I ran SPEC to see if there was any performance change. Performance difference was in the noise.

In D5550#10, @atrick wrote:

One thing I'm concerned about though is that I think you're using SCEVDivision for the first time in the standard LLVM pipeline (outside of the delinearizer). SCEVDivision looks like it recurses over the expression operands. Every time someone does this, we eventually have to track down some case that results in pathological compile time. Can you prove that this recursion will never visit the same expression twice? If not, I think you need to add a visited set as we do elsewhere, or find another way to check for exact division.

Thanks for the review.

Hi Andrew,

The review diff tool doesn't do a very good job of presenting it, but a big chunk of the patch is moving the class SCEVDivision and a few static functions earlier in the file so they can be used in ScalarEvolution::HowFarToZero.

LGTM

Committed as r213900.

Thanks! Test added. Will lead with that next time ;-)

Just a couple suggestions which might simplify the code.

Thanks! Committed with r213789.

Committed with r213412.

Thanks for the comments. I'll immediately follow this with the updated patch.

LGTM.

I added a warning if the wrong pragma unroll syntax is used with CUDA. When Tyler's change adding support for expressions in loop pragmas lands, I'll add a warning for the case when the pragma argument is not an integer literal in CUDA mode. Responses to Aaron's comments inline.

commited as r212782.

On Wed, Jun 25, 2014 at 9:34 AM, Eli Bendersky <eliben@google.com> wrote:

Is this part of the code well covered by tests? Maybe some more targeted
tests can be crafted.

Thanks for the comments. Responses are below. With this patch "#pragma unroll" and "#pragma clang loop unroll" syntaxes are both supported. I also added "#pragma nounroll" as both icc and xlc support this pragma.

Thanks all for your comments. This patch reduces much of the implementation duplication from the previous patch. Unroll and loop optimization hints now both share the same attribute.

Thanks for the comments, Eli. This patch addresses them all.