Page MenuHomePhabricator

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

Authored by hulx2000 on Sep 24 2015, 5:02 PM.

Details

Summary
This patch enable LoopReroll to reroll loop with pointer induction
variable, such as:

while (buf !=end ) {
   S += buf[0];
   S += buf[1];
   buf +=2;
};

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 -part 2/3.
hulx2000 updated this object.
hulx2000 set the repository for this revision to rL LLVM.
hfinkel added inline comments.Sep 25 2015, 4:10 PM
lib/Transforms/Scalar/LoopRerollPass.cpp
485 ↗(On Diff #35688)

dyn_cast -> cast (you've already tested IV->getType()->isPointerTy()).

513 ↗(On Diff #35688)

Don't you need an:

} else {
  break;
}

here. If the MulSCEV operands are a constant and some other expression, you don't always want to return the constant). Also, do you want to look through extensions and/or truncations here?

1340 ↗(On Diff #35688)

I realize it was like this before, but we should name this something more explanatory than H.

1342 ↗(On Diff #35688)

This is not really just 'One', how about we name it IncExpr, or IncrementExpr, etc.?

test/Transforms/LoopReroll/ptrindvar.ll
42 ↗(On Diff #35688)

We should also have a test case where the pointer operand is being decremented.

hulx2000 updated this object.
hulx2000 marked 4 inline comments as done.

Thanks Hal for your prompt comments, I have updated the patch.

Regards

Lawrence Hu

jmolloy requested changes to this revision.Sep 29 2015, 3:28 AM
jmolloy edited edge metadata.

Hi,

Thanks for working on this. I have plenty of comments :)

Cheers,

James

lib/Transforms/Scalar/LoopRerollPass.cpp
475 ↗(On Diff #35916)

This only works for pointers, not for integer steps, so can this be renamed to make that clear?

489 ↗(On Diff #35916)

Why declare NewSCEV here, instead of inside the if?

510 ↗(On Diff #35916)

I think this debug statement might not be that useful - can it be removed when committed?

511 ↗(On Diff #35916)

Braces {} around the else. if/elses should either both have braces or both not.

512 ↗(On Diff #35916)

Surely these breaks should be "return nullptr"? What if the first operand is a constant but the second is not an unknown? or is an unknown but !isSizeOf()? then currently you'll return the first operand but actually this method should fail.

528 ↗(On Diff #35916)

This would read better as:

if (!I->getType()->isIntegerTy() && !I->getType()->isPointerTy())
716 ↗(On Diff #35916)

It might read better if both these ifs were squashed together?

1360 ↗(On Diff #35916)

I really don't think "AddRecExpr" is any more helpful than "H". How about NewIVSCEV?

1361 ↗(On Diff #35916)

I don't like that these are declared outside of scope and initialized to nullptr - this isn't C, we can declare variables where we use them! :) Please sink these into the if expression (you can just use auto too)

1364 ↗(On Diff #35916)

You need to use cast<> instead of dyn_cast<> below. Instead, what you can do is this:

if (auto *PTy = dyn_cast<PointerTy>(Inst->getType())) {

then just use PTy below.

1371 ↗(On Diff #35916)

I realise it was like this before, but I don't actually think we need this to be of type SCEVAddRecExpr. I think const SCEV * should be sufficient, which means you can remove this cast.

1373 ↗(On Diff #35916)

I notice you've removed the compound braces around this but kept the comment... either keep the braces or remove the comment too, but give a rationale for removal if you're going to do that.

1386 ↗(On Diff #35916)

This nullptr initialization again... can we get rid of these? I mean, there seems to be no reason to even touch this line :/

1394 ↗(On Diff #35916)

This is disingenuous, as Minus1SCEV no longer holds minus one! The variable should be renamed.

1401 ↗(On Diff #35916)

Please replace the braces you've removed here. Mixing one-liners and braces isn't LLVM style.

This revision now requires changes to proceed.Sep 29 2015, 3:28 AM
hulx2000 marked 16 inline comments as done.Sep 30 2015, 6:01 PM

Thanks, James, most of your concerns are addressed

lib/Transforms/Scalar/LoopRerollPass.cpp
1361 ↗(On Diff #35916)

SizeOfExpr is used later, can't be declared in side if statement

hulx2000 edited edge metadata.
hulx2000 marked an inline comment as done.
jmolloy requested changes to this revision.Oct 21 2015, 2:09 AM
jmolloy edited edge metadata.

Hi Lawrence,

I've got some more style complaints, but nothing major.

Cheers,

James

lib/Transforms/Scalar/LoopRerollPass.cpp
385 ↗(On Diff #36169)

Please either document this or move it to the protected: section below where all the helpers are.

477 ↗(On Diff #36169)

I don't think it's very useful to have this CIncSCEV variable. It'd be a lot neater (and more up to coding style) if instead of "CIncSCEV = ..; ...; return CIncSCEV;", just "return CIncSCEV;"

491 ↗(On Diff #36169)
return dyn_cast<SCEVConstant>(...);
492 ↗(On Diff #36169)

There should either be braces around both the if and else or around neither of them.

493 ↗(On Diff #36169)

return dyn_cast<SCEVConstant>(...)

504 ↗(On Diff #36169)

Braces or no braces, not mixed braces.

1386 ↗(On Diff #36169)

Please assert(SizeOfExpr); here, to ensure it's initialized (if someone changes the if() above!)

This revision now requires changes to proceed.Oct 21 2015, 2:09 AM
hulx2000 marked 5 inline comments as done and an inline comment as not done.Oct 26 2015, 2:12 PM
hulx2000 added inline comments.
lib/Transforms/Scalar/LoopRerollPass.cpp
477 ↗(On Diff #36169)

CIncSCEV is still needed later but I reduced the lifespan of it.

489 ↗(On Diff #36169)

I didn't change this, The purpose of declaring NewSCEV here just to break the long statement into two shorter ones, and compiler will generate temp anyway even if I don't that.

hulx2000 edited edge metadata.
hulx2000 marked an inline comment as not done.
hulx2000 edited edge metadata.

clang-format it

hfinkel accepted this revision.Dec 10 2015, 5:46 PM
hfinkel edited edge metadata.

LGTM.

We should really get this enabled by default for functions when optimizing for size. Care to take a stab at that?

lib/Transforms/Scalar/LoopRerollPass.cpp
1370 ↗(On Diff #38553)

No space after first "

jmolloy accepted this revision.Dec 15 2015, 12:49 AM
jmolloy edited edge metadata.

Sorry, I didn't realise that according to phab I was blocking this. I saw Hal's LGTM and ignored it.

This revision is now accepted and ready to land.Dec 15 2015, 12:49 AM
hulx2000 edited edge metadata.

Hi, Guys:

Very sorry that it took too long before I get back to this patch due to our release related work, now I need to rebase the code and fix the checking pattern of the testcase, could you please take a look again and give me a LGTM again? This time it will merged quickly.

Nice day.

Regards

Lawrence Hu

fix a minor format issue

Remove one space spotted by Hal

hulx2000 marked an inline comment as done.Jan 22 2016, 5:06 PM

Still LGTM; please proceed.

Thanks a lot for your quick response, Hal.

Have a nice day.

Regards

Lawrence Hu

Closed by commit rL258700 (authored by lawrence). · Explain WhyJan 25 2016, 10:57 AM
This revision was automatically updated to reflect the committed changes.