[llvm] [SystemZ] Improve shouldCoalesce() for i128. (PR #74942)

Jonas Paulsson via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 12 12:56:53 PST 2023


JonPsson1 wrote:

> I don't really think this is a concern - if this happens, then regalloc may have to insert a move at some CFG edge, but that should work fine. The underlying problem requiring this special handling really was related to local problems with register availability around a call site and/or inline asm, I think.

OK - if we trust regalloc to do that kind of splitting, this seems like a good improvement to me. I guess in any case we were never truly on safe ground as the scheduler is run after coalescing and could potentially move some inlineasms so as to make more physreg clobbers overlap a GPR128 interval.

Patch updated to allow coalescing even with a global GR128 as long as the subreg is local. I thought checking the local part of the GPR128 interval as well as the subreg region for clobbering physregs would be desired, but it was not as trivial as I had hoped for to find out if the GPR128 interval is live in/out or if not what is the first/last point of liveness of it in MBB. I think my current version is ok, but perhaps there is a better way? (If you think it would be enough to only check the subreg range, ofc that would be much simpler...)

A more simple way for me at least would be to use MRI and iterate over uses (/defs) and by comparing slot indexes between them finding the latest (/earliest) instruction. liveAt() could be used to check for liveness around MBB boundaries.

One thing I am aware of is that there could with this be a hole in the liveness, if a GPR128 (defined by a COPY) is live for a while, then killed, and then the same vreg is redefined later in MBB and becomes live. This patch would then only scan to the first kill. My hope is then that regalloc would if needed split the interval also at that point, but ofc I am not sure about that...

I guess this might be unnecessarily complicated. Maybe it would be better to make sure we don't emit these COPYs (with i128 in VRs) in the first place and then leave this method as it was for older machines..? If that could be done with a pseudo or so, that is.



https://github.com/llvm/llvm-project/pull/74942


More information about the llvm-commits mailing list