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

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa marked 3 inline comments as done.
jonpa added inline comments.


================
Comment at: lib/Target/SystemZ/SystemZRegisterInfo.cpp:163
+  // regalloc running out of registers. That may happen as long as subreg
+  // liveness is turned off per default due to being compile time expensive.
+  if (SrcRC->hasSubClassEq(&SystemZ::GR128BitRegClass) &&
----------------
uweigand wrote:
> While turning subreg liveness  on fixes the simpler test case you have here, the even more restricted test case I've added to the bugzilla actually still fails even then.  So I'm not sure we should mention subreg liveness here as the reason why this is needed.
I see - removed.


================
Comment at: lib/Target/SystemZ/SystemZRegisterInfo.cpp:166
+      getRegSizeInBits(*DstRC) <= 64)
+    return false;
+
----------------
uweigand wrote:
> Should this be symmetrical?  I.e. something like
> 
> ```
>   if (NewRC->hasSubClassEq(&SystemZ::GR128BitRegClass) &&
>       (getRegSizeInBits(*SrcRC) <= 64 || getRegSizeInBits(*DstRC) <= 64))
>     return false;
> 
> ```
Do we need a test case for this also?


================
Comment at: test/CodeGen/SystemZ/regalloc-GR128.ll:6
+define i64 @test(i64 %dividend, i64 %divisor) {
+; CHECK-NOT: LLVM ERROR: ran out of registers during register allocation
+init:
----------------
uweigand wrote:
> Wouldn't just running llc and checking its exit code already test that there is no LLVM ERROR?
OK - I thought it was a nice "comment", but I guess we already have a comment at the top of the file.


================
Comment at: test/CodeGen/SystemZ/regalloc-GR128.ll:9
+  %rem = urem i64 %dividend, %divisor
+  call void asm sideeffect "", "{r1},{r3},{r5},{r7},{r9},{r11},{r12},{r13}"(i64 0, i64 0, i64 0, i64 0, i64 0, i64 0, i64 0, i64 %rem)
+  ret i64 %rem
----------------
uweigand wrote:
> Maybe better to use the more restricted test from the bugzilla.
yes.


https://reviews.llvm.org/D37899