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

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
469

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

497

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?

1384

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

1386

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

test/Transforms/LoopReroll/ptrindvar.ll
43

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
459

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

473

Why declare NewSCEV here, instead of inside the if?

494

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

495

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

496

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.

510

This would read better as:

if (!I->getType()->isIntegerTy() && !I->getType()->isPointerTy())
697

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

1384

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

1385

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)

1388

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.

1395

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.

1397

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.

1410

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

1418

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

1425

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
1385

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
369

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

461

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;"

475
return dyn_cast<SCEVConstant>(...);
476

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

477

return dyn_cast<SCEVConstant>(...)

488

Braces or no braces, not mixed braces.

1415

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
461

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

473

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

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.