[PATCH] D37899: [SystemZ] Implement shouldCoalesce() to help regalloc to avoid running out of registers with GR128 regs

Mon Sep 25 05:05:24 PDT 2017

jonpa updated this revision to Diff 116522.
jonpa added a comment.
Herald added subscribers: javed.absar, nhaehnle, arsenm.

Patch updated.

> I looked into making i128 legal when I recently added the 128-bit atomics, but in the end decided against it. Not only would it be a lot of work (since you basically would have to reimplement all the operations on i128 that are currently handled by common code), but it is not clear that we actually always want it.

I thought one could get away with adding calling setOperationAction(..., i128, Expand) on all operations that we do not support...

> i128 lives in a even/odd register pair, but this is really only needed for the few special instructions that operate on those pairs. If you had just a normal load followed by a normal store of a i128 value, you *want* to break it into two independent i64 copies -- there is no reason at all to constrain the register allocator to use a even/odd pair here.

I would have expected that for a tiny live range, it is always better to avoid extra COPYs.

> Another option might be to not *completely* prevent coalescing a 128-bit pair with its component registers, but allow it in cases where there is no excessive register pressure. But I'm not sure exactly how to get at this information at the time we make the coalescing decision ...

There is a current discussion relating to the mischeduler register pressure tracking about the fact that there is no real support to query global liveness yet. It may be that this will get added soon, and perhaps then some cheap register pressure estimate might be available.

Until then, I thought it might serve well to at least allow coalescing in cases of tiny, local live-ranges, e.g. loading and then storing 128 bits.

The updated patch uses LiveIntervals to quickly check if the two connected vregs are local to MBB and what the first and last instructions are (it may be possible to do this without LIS if we are willing to scan the whole MBB). I merely tried some values of 10 instructions and 8 phys-reg operands in order to detect a non-safe live-range. It may be that we want to check for vreg operands as well.

I don't have adequate tests to tune this further, but this might hopefully be a good starting point. At least all the regressions we saw before are now gone, while still handling your test case.

On SPEC, I see

Current patch:

                 master                 patched
  lg             : 320549               318515                -2034
  stg            : 125311               124230                -1081
  lr             :  25927                26632                 +705
  lgr            : 343901               343602                 -299
  llill          :    982                  869                 -113
  c              :  10989                11061                  +72

  "Spill|Reload"   168062               165145                -2917

https://reviews.llvm.org/D37899

Files:
  include/llvm/Target/TargetRegisterInfo.h
  lib/CodeGen/RegisterCoalescer.cpp
  lib/Target/AMDGPU/SIRegisterInfo.cpp
  lib/Target/AMDGPU/SIRegisterInfo.h
  lib/Target/ARM/ARMBaseRegisterInfo.cpp
  lib/Target/ARM/ARMBaseRegisterInfo.h
  lib/Target/SystemZ/SystemZRegisterInfo.cpp
  lib/Target/SystemZ/SystemZRegisterInfo.h
  test/CodeGen/SystemZ/regalloc-GR128.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D37899.116522.patch
Type: text/x-patch
Size: 9302 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170925/72f25c2a/attachment.bin>