Page MenuHomePhabricator

[LiveRegUnits] Add phys_regs_and_masks iterator range (NFC).
ClosedPublic

Authored by fhahn on Nov 21 2019, 12:45 PM.

Details

Summary

This iterator range just includes physical registers and register masks,
which are interesting when dealing with register liveness.

Diff Detail

Event Timeline

fhahn created this revision.Nov 21 2019, 12:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 21 2019, 12:45 PM
paquette added inline comments.Dec 6 2019, 4:16 PM
llvm/lib/CodeGen/LivePhysRegs.cpp
47

In this patch, we don't have an equivalent check to O->isDebug(). Instead, all we have is a check for MOP.isDef().

Do we need a similar check in the new code?

fhahn updated this revision to Diff 232901.Dec 9 2019, 11:07 AM

Filter out debug operands and add doxygen comment to function.

fhahn marked an inline comment as done.Dec 9 2019, 11:09 AM
fhahn added inline comments.
llvm/lib/CodeGen/LivePhysRegs.cpp
47

Yep, I originally thought that Debug operands cannot also be physical registers, but given that all the existing places check for it I guess its safer to explicitly filter them out. I've update the patch ( and also added a comment to phys_regs_and_masks)

Build result: pass - 60637 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

paquette accepted this revision.Dec 10 2019, 2:37 PM

LGTM!

This revision is now accepted and ready to land.Dec 10 2019, 2:37 PM
This revision was automatically updated to reflect the committed changes.
jinlin added a subscriber: jinlin.EditedDec 1 2020, 9:43 AM

Hi Florian,

I have encountered a big compile time regression in the machine outlining caused by https://reviews.llvm.org/D70562.

After I backed out the change in D70562, the compile time for the three apps below was back.

I believe the reason is due to the introduction of function phys_regs_and_masks as below. Originally there is only one loop. With this change, it becomes two loops since it collects the legal operands first. If the filter does not have any effect, it essentially means the loop trip count is doubled.

for (ConstMIBundleOperands O(MI); O.isValid(); ++O) {

>

for (const MachineOperand &MOP : phys_regs_and_masks(MI)) {

When I back out the change D70562, I have also made minor changes in file llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp since phys_regs_and_masks is used in that file.

app1                 app2               app3

Build Time (LLVM 9.0) 4278.512s 2547.800s 5917.305s

Build Time (LLVM 10.0) 5625.795s 4254.092s 9474.335s

Build Time (LLVM 10.0 + backout D70562) 4757.243s (+15%) 2618.070s (+42%) 6996.272s (+26%)

fhahn added a comment.Dec 2 2020, 12:26 PM

Hi Florian,

I have encountered a big compile time regression in the machine outlining caused by https://reviews.llvm.org/D70562.

After I backed out the change in D70562, the compile time for the three apps below was back.

I believe the reason is due to the introduction of function phys_regs_and_masks as below. Originally there is only one loop. With this change, it becomes two loops since it collects the legal operands first. If the filter does not have any effect, it essentially means the loop trip count is doubled.

for (ConstMIBundleOperands O(MI); O.isValid(); ++O) {

>

for (const MachineOperand &MOP : phys_regs_and_masks(MI)) {

When I back out the change D70562, I have also made minor changes in file llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp since phys_regs_and_masks is used in that file.

app1                 app2               app3

Build Time (LLVM 9.0) 4278.512s 2547.800s 5917.305s

Build Time (LLVM 10.0) 5625.795s 4254.092s 9474.335s

Build Time (LLVM 10.0 + backout D70562) 4757.243s (+15%) 2618.070s (+42%) 6996.272s (+26%)

Let me take a look. Are the apps accessible somewhere? Or could be you provide a MIR/bitcode file to reproduce?

Sorry I could not provide bitcode file since it is related to IP issue.

Let me take a look. Are the apps accessible somewhere? Or could be you provide a MIR/bitcode file to reproduce?