Page MenuHomePhabricator

Minor fixes in Loop Strength Reduction
ClosedPublic

Authored by evstupac on Nov 18 2016, 5:01 PM.

Details

Summary

The patch contains the following minor fixes:

  1. Function renaming: from specific "Condition Instruction" to general "Instruction".
  2. Debug print: form address print to Instruction dump.
  3. Set count of first register in best registers search.

Diff Detail

Event Timeline

evstupac updated this revision to Diff 78605.Nov 18 2016, 5:01 PM
evstupac retitled this revision from to Minor fixes in Loop Strength Reduction.
evstupac updated this object.
evstupac added reviewers: sanjoy, jonpa.
evstupac set the repository for this revision to rL LLVM.
evstupac added subscribers: llvm-commits, sanjoy, jonpa.
anna added a subscriber: anna.Nov 18 2016, 7:19 PM

The second and third changes look good. Is there a reason for the first one?

The second and third changes look good. Is there a reason for the first one?

Anna, thanks for taking a look.
Regarding 1st change.
It just widen the function usability.
Function "FindIVUserForCond" is general and could be used not only for "Conditions". It can do the same search for any Instruction.
I want to reuse it in one of my future commits for something more than ICmpInst.

anna added a comment.Nov 19 2016, 10:20 AM

Evgeny ,
This looks good to me, but I'll leave it to others to explicitly LGTM.

You can add Quentin as well as reviewer (he's the new code owner of LSR).

hfinkel added inline comments.
lib/Transforms/Scalar/LoopStrengthReduce.cpp
2818 ↗(On Diff #78605)

This debugging-statement change LGTM; please commit this separately.

4149 ↗(On Diff #78605)

Do you have a test case for this change?

If/When you commit this, it should be separate from the other changes.

evstupac added inline comments.Nov 20 2016, 11:49 AM
lib/Transforms/Scalar/LoopStrengthReduce.cpp
4149 ↗(On Diff #78605)

No. I don't. I can try to create.
However, it looks like a misprint in MAX search algorithm. We are always missing the case when first Reg in RegUses has the biggest RegUses.getUsedByIndices(...).count().

hfinkel added inline comments.Nov 20 2016, 12:32 PM
lib/Transforms/Scalar/LoopStrengthReduce.cpp
4149 ↗(On Diff #78605)

Okay; makes sense to me.

evstupac updated this revision to Diff 78784.Nov 21 2016, 2:18 PM

"2. Debug print: ..." committed separately.

wmi added a subscriber: wmi.Nov 21 2016, 3:26 PM
evstupac added inline comments.Nov 22 2016, 2:25 PM
lib/Transforms/Scalar/LoopStrengthReduce.cpp
4149 ↗(On Diff #78605)

I'm I right that I can go ahead and commit this part?
Or we still need a test case?
Wei Mi gave me a test case, so I can add it here. But I don't think that for the changes like this we need a test case in LIT tests.

PING.
Is it ok to fix MAX calculation?

qcolombet accepted this revision.Nov 30 2016, 12:18 PM
qcolombet edited edge metadata.

Hi,

LGTM.

  1. Function renaming: from specific "Condition Instruction" to general "Instruction".

I would wait for that change to when we actually need it though.

Cheers,
-Quentin

This revision is now accepted and ready to land.Nov 30 2016, 12:18 PM

Committed r288278

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 6:38 AM
Herald added a subscriber: hiraditya. · View Herald Transcript