This is an archive of the discontinued LLVM Phabricator instance.

Early expansion of MUX instructions on Hexagon
ClosedPublic

Authored by kparzysz on Mar 13 2015, 1:18 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

kparzysz updated this revision to Diff 21950.Mar 13 2015, 1:18 PM
kparzysz retitled this revision from to Early expansion of MUX instructions on Hexagon.
kparzysz updated this object.
kparzysz edited the test plan for this revision. (Show Details)
kparzysz added a reviewer: qcolombet.
kparzysz set the repository for this revision to rL LLVM.
kparzysz added a subscriber: Unknown Object (MLST).
qcolombet edited edge metadata.Mar 16 2015, 4:25 PM

Hi Krzysztof,

Could you comment the purpose of each function?

Right now, I do not understand for instance why after you have shrunk the live-ranges to the uses, you re-extend them again to the dead defs.
I believe this becomes clear when you explain the situation you are fixing in each function :).

Thanks,
-Quentin

Hi Quentin. Sorry, I missed your earlier comment. Sure, I'll add comments.

kparzysz updated this revision to Diff 22344.Mar 20 2015, 7:26 AM
kparzysz edited edge metadata.

Added comments to non-self-explanatory functions.

Hi Krzysztof,

Thanks for your patience.

The code that updates the liveness seems good to me. You have duplicated some functionality from MachineInstr that I pointed out. Moreover, I believe more comments on the functions themselves would be general goodness.

I admit I have been less careful for the end of the review (starting at HexagonExpandCondsets::removeInstrFromLiveness), since you showed you understand the underlying implications on the liveness.
That being said, make sure that the test cases you are adding cover all the liveness corner cases you handle. Right now, I am a bit dubious that you do not need more of them. You know more than me how all that is being invoked, though :).

Note: I haven’t check at all the logic for the optimization itself.

Cheers,
-Quentin

lib/Target/Hexagon/HexagonExpandCondsets.cpp
259

I think this function deserves a comment.
From the name, it is not clear that this update the undef flag of the uses of Reg at S.

265

You can use for-range based loop for that.

300

This if is probably worth for performances than always setting the flag to false.

329

Ditto.

347

This functionality is already available via MachineInstr::clearKillInfo().

354

This method would deserve a comment as well and/or a renaming.

> You mark all the uses as kill.

371

Would deserve a comment.

In particular explain that you create holes in the live-range and how do you plan to fix them.

452

Okay, I see where you fill the holes now :).

528

Not necessarily.

Anyway, use MachineInstr::clearRegisterKills.

kparzysz updated this revision to Diff 22664.Mar 25 2015, 1:04 PM

Added more comments, replaced uses of MIOperands with range-based loops, replaced redundant functions with calls to equivalent functions from MachineInstr.

Inline comment.

lib/Target/Hexagon/HexagonExpandCondsets.cpp
89

This is a MachineFunction pass. In runOnMachineFunction you can set these variables. Just look at how the other MachineFunction passes do it.

kparzysz updated this revision to Diff 22680.Mar 25 2015, 2:34 PM

Get the subtarget info from MF, not TM.

LGTM for the update liveness part.

I cannot comment on the hexagon-specific aspect of the patch.

Cheers,
-Quentin

lib/Target/Hexagon/HexagonExpandCondsets.cpp
257

We usually do not use hyphens in comments.

Submitted in r233696. Thanks all for comments!

lib/Target/Hexagon/HexagonExpandCondsets.cpp
257

Removed all word breaks from comments

259

Will do.

265

Will convert all such cases to range-based loops.

300

Will remove the if check.

329

Same here.

347

Will do.

354

I changed the name to updateKillFlags and added comment explaining the functionality.

528

Will use clearRegisterKills.

kparzysz accepted this revision.Mar 31 2015, 6:45 AM
kparzysz added a reviewer: kparzysz.
This revision is now accepted and ready to land.Mar 31 2015, 6:45 AM
kparzysz closed this revision.Jan 8 2016, 7:44 AM