Diff Detail
- Repository
- rL LLVM
Event Timeline
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 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. | |
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. |
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. |
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. |
This is a MachineFunction pass. In runOnMachineFunction you can set these variables. Just look at how the other MachineFunction passes do it.