This is an archive of the discontinued LLVM Phabricator instance.

Expand loop reroll to handle loop with multiple induction variables and negative increment -part 1/3
ClosedPublic

Authored by hulx2000 on Jun 16 2015, 10:38 AM.

Details

Summary
Handle loop with negtive induction variable increment

This patch extends LoopReroll pass to hand the loops which
are similar to the following:

      while (len > 1) {
            sum4 += buf[len];
            sum4 += buf[len-1];
            len -= 2;
        }

Basically, negative loop increment.

The original patch will be split into 3, that's the first one.

Diff Detail

Repository
rL LLVM

Event Timeline

hulx2000 retitled this revision from to Expand loop reroll to handle loop with multiple induction variables and negative increment.
hulx2000 updated this object.
hulx2000 edited the test plan for this revision. (Show Details)
hulx2000 added reviewers: jmolloy, hfinkel.
hulx2000 set the repository for this revision to rL LLVM.
hulx2000 added subscribers: apazos, zinob.
hulx2000 added a subscriber: Unknown Object (MLST).Jun 16 2015, 11:33 AM

handle some of Ana's comments

hfinkel edited edge metadata.Jun 22 2015, 6:07 PM

Please enhance your patch description to explain, not just what he patch enables (which you've done), but generically how it does it. There is a bunch of new code here, and it will be good to get a high-level overview of what it does (and I'd expect such a description to appear in the commit message anyway, so this is not "just for review").

Please upload the patch with full context, see:

http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

for instructions.

lib/Transforms/Scalar/LoopRerollPass.cpp
447

We generally abbreviate Instruction to Inst, not Insn. Please rename this function.

Also, please comment what this function does. It is certainly more than just determining if the instruction is a comparison of some kind.

hulx2000 updated this object.
hulx2000 edited edge metadata.
hulx2000 removed rL LLVM as the repository for this revision.

Thanks, Hal, sorry for the delay, just saw your comments this morning

jmolloy requested changes to this revision.Jul 6 2015, 5:11 AM
jmolloy edited edge metadata.

Hi,

I'm afraid there are too many different changes bundled together here for me to really review. Please could you split this out into:

  1. Adding negative offset support
  2. Adding pointer induction variable support
  3. Adding multiple induction variable support.

Cheers,

James

This revision now requires changes to proceed.Jul 6 2015, 5:11 AM

Hi, James:

Thank you very much for your comment.

Those patches are bundled together because they are strongly correlated, for an example, if I add support for pointer induction variable, I must support multiple induction variable, otherwise a lot of test will be broken.

I will see if I can split it into three with some extra safety check.

Regards

Lawrence Hu

hulx2000 retitled this revision from Expand loop reroll to handle loop with multiple induction variables and negative increment to Expand loop reroll to handle loop with multiple induction variables and negative increment -part 1/3.
hulx2000 updated this object.
hulx2000 edited edge metadata.
hulx2000 set the repository for this revision to rL LLVM.
hulx2000 edited edge metadata.

remove white spaces

upload the full context diff

jmolloy requested changes to this revision.Jul 15 2015, 12:38 AM
jmolloy edited edge metadata.

Hi,

Thanks for making these changes - it looks much better now. A few comments still.

Cheers,

James

lib/Transforms/Scalar/LoopRerollPass.cpp
500

I think this line and line 503 can both be merged, and put on line 505. There's no need to ZExt this one and SExt the other - both can be SExted.

503

This can be much simpler. You can just use APInt::abs():

AInt = AInt.abs();
if (AInt.isZero() || AInt.uge(MaxInc))
  continue;
IVToIncMap[I] = AInt.getSExtValue();
668

I'm not sure how adding GEP support here relates to negative values. Was this an accidentally added hunk?

713

I think you need to use std::abs() above, on line 721 too. Otherwise the guard on line 721 is useless.

1268

Double semicolon (;;).

Also, you can squash this into just one statement:

bool Negative = IVToIncMap[IV] < 0;
This revision now requires changes to proceed.Jul 15 2015, 12:38 AM
hulx2000 added inline comments.Jul 15 2015, 10:39 AM
lib/Transforms/Scalar/LoopRerollPass.cpp
668

Oh, sorry, this is left over when I split my patch, it is require for pointer induction variable though, but I can remove it for now.

Will fix the rest, thanks.``

hulx2000 edited edge metadata.

Thanks, James for your valuable comments.

I have updated my patch.

Hi,

This is looking a lot better, thanks!

James

lib/Transforms/Scalar/LoopRerollPass.cpp
373

This should be const, to notify a user that it is not captured or modified.

438

Please add a docstring, like all other variables.

666

I think this is still relating to adding GEP support, is it not?

test/Transforms/LoopReroll/negative.ll
17

This test will fail in release builds because registers will lose their names. Please either make it not rely on named variables or add a

; REQUIRES: asserts

to the top (please also check the line above is actually correct, I've just pasted it from memory!)

hulx2000 edited edge metadata.
hulx2000 marked 10 inline comments as done.

Thanks Jame.

Sorry that missed a few thing last time, this time should be better.

Best, I didn't change the const thing, compiler complains, and I didn't see testcase asset in release build, I tried pass, it is fine, are yours a special build?

Regards

Lawrence Hu

hulx2000 added inline comments.Jul 17 2015, 10:34 AM
lib/Transforms/Scalar/LoopRerollPass.cpp
373

unfortunately, compiler always contains if I do that.

jmolloy accepted this revision.Jul 17 2015, 1:50 PM
jmolloy edited edge metadata.

LGTM, thanks for making those changes.

If you don't add REQUIRES: asserts, you'll get complaints on the list when this goes in. I forget exactly which cmake variable you change to remove value names completely, but rest assured there are people who use it - I've found that out myself :)

Cheers,

James

This revision is now accepted and ready to land.Jul 17 2015, 1:50 PM

Great, Thanks, James.

I will add Assert from the next test and on.

Would you mind to commit it for you so I can start the next one? I don't have commit right yet.

mzolotukhin added inline comments.
lib/Transforms/Scalar/LoopRerollPass.cpp
169–172

s/it's/its/

172

I'd rather commit this typo fix immediately when I found it (no pre-commit review is required for this kind of changes).

438

s/it's/its/

665

No need to add a blank line here.

1267

Redundant whitespaces.

test/Transforms/LoopReroll/negative.ll
20–28

Do we really need to check that we generate *exactly* these statements? Would it be possible to look for, say, a specific branch (or absence of the branch) - this would make the test much more stable.

30–39

Do we really need to have this body, or would the test work with almost empty loop?
Like this:

while (len > 1) {
            len -= 2;
        }

instead of

while (len > 1) {
            sum4 += buf[len];
            sum4 += buf[len-1];
            len -= 2;
        }

Since you only run loop-reroll, it shouldn't be collapsed to nop.

54–55

Maybe just return i32 %sum4.0.lcssa to get rid of the goo function?

hulx2000 marked 2 inline comments as done.Jul 21 2015, 6:51 PM
hulx2000 added inline comments.
test/Transforms/LoopReroll/negative.ll
30–39

The simple loop won't do enough test for the logic in LoopReroll

hulx2000 edited edge metadata.

Addressed Michael's comment

hulx2000 marked 3 inline comments as done.Jul 22 2015, 10:39 AM
hulx2000 marked 6 inline comments as done.Jul 22 2015, 10:40 AM

Hi, Michael

Could you please accept it if you have now further comment? It will be easier for me to prepare next two patches.

Thanks.

Lawrence Hu

Hi, Hal:

Do you have any other comments about this patch, could you please accept it if not?

Thanks

Lawrence Hu

hfinkel added inline comments.Jul 24 2015, 5:19 AM
lib/Transforms/Scalar/LoopRerollPass.cpp
1275–1276

Line is too long.

1294–1295

Line is too long.

test/Transforms/LoopReroll/negative.ll
3

Remove this. There is no valid reason why a test like this needs to be asserts-mode only. And it is really important that we have as much testing as possible for the NDEBUG builds (even if that were to require using more regexes in the test). That having been said, what James said was not quite right. The NDEBUG opt does not remove all variable names (certain passes did that, or used to, but that's another matter). What is true is that NDEBUG Clang does not produce variable names for locals.

Thanks, Hal,

Updated.

Regards

hfinkel accepted this revision.Jul 24 2015, 10:23 AM
hfinkel edited edge metadata.

LGTM now too, thanks!

hulx2000 closed this revision.Jul 24 2015, 3:27 PM

committed as git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@243171 91177308-0d34-0410-b5e6-96231b3b80d8