Page MenuHomePhabricator

[LSR] canonicalize Prod*(1<<C) to Prod<<C
ClosedPublic

Authored by jingyue on Jun 15 2015, 10:48 AM.

Details

Summary

Because LSR happens at a late stage where mul of a power of 2 is
typically canonicalized to shl, this canonicalization emits code that
can be better CSE'ed.

Diff Detail

Event Timeline

jingyue updated this revision to Diff 27688.Jun 15 2015, 10:48 AM
jingyue retitled this revision from to [LSR] canonicalize Prod*(1<<C) to Prod<<C.
jingyue updated this object.
jingyue edited the test plan for this revision. (Show Details)
jingyue added reviewers: sanjoy, atrick.
jingyue added a subscriber: Unknown Object (MLST).
jingyue added inline comments.Jun 15 2015, 10:49 AM
lib/Analysis/ScalarEvolutionExpander.cpp
792

Btw, visitUDivExpr does similar canonicalization too.

majnemer added inline comments.
lib/Analysis/ScalarEvolutionExpander.cpp
771–774

You could write this most concisely as:

// Canonicalize Prod*(1<<C) to Prod<<C.
if (match(W, m_Power2()) {
  ...
}
771–774

s/most/more

jingyue updated this revision to Diff 27706.Jun 15 2015, 12:09 PM

Simplify the code with match(m_Power2). Thanks David!

sanjoy edited edge metadata.Jun 18 2015, 7:05 PM

I don't see anything wrong with this change, but I also do not know LSR well enough to LGTM it. I suppose you'll have to wait for Andy to chime in.

majnemer added inline comments.Jun 23 2015, 10:52 PM
lib/Analysis/ScalarEvolutionExpander.cpp
31–32

I'd add using namespace PatternMatch; here.

772

With the using declaration, we can remove the scope qualifiers here.

772–778

Seeing the continue here is a little confusing, can we just make it an if/else?

774–775

I'd use Constant::getIntegerValue instead as it handles the case where Ty is a vector type.

jingyue updated this revision to Diff 28365.Jun 24 2015, 10:29 AM
jingyue edited edge metadata.

using namespace PatternMatch and some other minor changes

Thanks for the review, David.

lib/Analysis/ScalarEvolutionExpander.cpp
31–32

done

772

done

772–778

done

774–775

Vector types are not SCEVable, so SCEV::getType() never returns a vector type. But it doesn't harm to add an assert.

majnemer accepted this revision.Jun 24 2015, 10:35 AM
majnemer added a reviewer: majnemer.

LGTM

This revision is now accepted and ready to land.Jun 24 2015, 10:35 AM
atrick accepted this revision.Jun 24 2015, 10:41 AM
atrick edited edge metadata.

Sorry I dropped this review. You change is fine with me.

jingyue closed this revision.Jun 24 2015, 12:32 PM