Page MenuHomePhabricator

Fix bug 23851: Preserve metadata for the unswitched branch in loop-unswitch
ClosedPublic

Authored by weimingz on Jun 15 2015, 1:20 PM.

Details

Summary

Currently, when the branch is unswitched, the newly created branch does not copy the metadata (like branch_weights) from the old branch.

Diff Detail

Event Timeline

weimingz updated this revision to Diff 27709.Jun 15 2015, 1:20 PM
weimingz retitled this revision from to Fix bug 23851: Preserve metadata for the unswitched branch in loop-unswitch.
weimingz updated this object.
weimingz edited the test plan for this revision. (Show Details)
weimingz added a subscriber: Unknown Object (MLST).
reames requested changes to this revision.Jun 17 2015, 1:13 PM
reames added a reviewer: reames.
reames added a subscriber: reames.

Can you upload a full context diff please? It's hard to assess what you're changing with the current limited context.

The general direction seems workable.

A couple of questions:

  1. Are we sure the direction of the branches are the same? If not, we may need to invert the profile metadata.
  2. Should we passing in the original terminator? Or just information about the metadata? Once I have context, I may have suggestions in this area.
  3. You should white list the metadata. Not all metadata will neccessarily apply.
This revision now requires changes to proceed.Jun 17 2015, 1:13 PM
weimingz updated this revision to Diff 27873.Jun 17 2015, 2:35 PM
weimingz edited edge metadata.

full diff of the original patch

HI Philip,

Thanks for reviewing and the suggestions. I will check the direction and white list the metadata.

Weiming

Can you upload a full context diff please? It's hard to assess what you're changing with the current limited context.

The general direction seems workable.

A couple of questions:

  1. Are we sure the direction of the branches are the same? If not, we may need to invert the profile metadata.
  2. Should we passing in the original terminator? Or just information about the metadata? Once I have context, I may have suggestions in this area.
  3. You should white list the metadata. Not all metadata will neccessarily apply.
weimingz updated this revision to Diff 27914.Jun 17 2015, 10:30 PM
weimingz edited edge metadata.
  1. White list the metadata to be copied. Currently, prof and dbg nodes are copied.
  2. Handle the case if True/False is swapped in the new branch
  3. I prefer passing "Instruction*" instead of "SmallVectorImpl< std::pair< unsigned, MDNode * >> &MDs" because 1) the latter is much longer 2) each call site needs to prepare the vector and call getAllMetadata().
hfinkel added inline comments.
lib/Transforms/Scalar/LoopUnswitch.cpp
693

Please name this

copyMetadata

the convention is not to capitalize the d.

720

I'd prefer there be a

// fallthrough

comment here.

weimingz updated this revision to Diff 28185.Jun 22 2015, 6:18 PM

copyMetaData -> copyMetadata
and add comments for the fallthrough case

hfinkel accepted this revision.Jun 22 2015, 6:21 PM
hfinkel added a reviewer: hfinkel.

LGTM.

Thank you, Hal!

mcrosier accepted this revision.Aug 5 2015, 7:46 AM
mcrosier edited edge metadata.

Committed in r240378.

@reames: Can you please close this revision? I'm unable to do so because of your -1.

reames accepted this revision.Aug 5 2015, 2:12 PM
reames edited edge metadata.
This revision is now accepted and ready to land.Aug 5 2015, 2:12 PM
reames closed this revision.Aug 5 2015, 2:12 PM