This is an archive of the discontinued LLVM Phabricator instance.

When looking for a spill slot in reg scavenger, find one that matches RC
ClosedPublic

Authored by kparzysz on May 16 2016, 12:47 PM.

Details

Summary

When looking for an available spill slot, the register scavenger would stop after finding the first one with no register assigned to it. That slot may have size and alignment that do not meet the requirements of the register that is to be spilled. Instead, find an available slot that is the closest in size and alignment to one that is needed to spill a register from RC.

Diff Detail

Repository
rL LLVM

Event Timeline

kparzysz updated this revision to Diff 57384.May 16 2016, 12:47 PM
kparzysz retitled this revision from to When looking for a spill slot in reg scavenger, find one that matches RC.
kparzysz updated this object.
kparzysz added a reviewer: qcolombet.
kparzysz set the repository for this revision to rL LLVM.
kparzysz added a subscriber: llvm-commits.
kparzysz updated this revision to Diff 57394.May 16 2016, 1:55 PM
kparzysz updated this object.
qcolombet added inline comments.May 17 2016, 2:24 PM
lib/CodeGen/RegisterScavenging.cpp
414 ↗(On Diff #57394)

Did you actually see cases where we should care?

I would rather return the first matching one and be done with the loop.

439 ↗(On Diff #57394)

Given that we saw it can happen, I would go with a report_fatal_error.

test/CodeGen/Hexagon/reg-scavenger-valid-slot.ll
3 ↗(On Diff #57394)
  • Run opt instnamer on the input.
  • Add check lines.
  • Add a comment on what this test is exercising.
106 ↗(On Diff #57394)

Could you make the test smaller, e.g., via using inline asm to clobber registers?
(You could also use mir input.)

kparzysz added inline comments.May 17 2016, 2:45 PM
lib/CodeGen/RegisterScavenging.cpp
414 ↗(On Diff #57394)

On Hexagon we can reserve spill slots for vectors (64-byte size/alignment) and integer registers (4/8-byte size/alignment). I'm concerned that depending on the order of reservation we can end up using a vector slot for an integer register.

439 ↗(On Diff #57394)

Will do.

test/CodeGen/Hexagon/reg-scavenger-valid-slot.ll
106 ↗(On Diff #57394)

Let me see what I can do with inline asm. I haven't tried MIR with our backend so I'm not sure how much work it will be to get it to work.

kparzysz marked 2 inline comments as done.May 17 2016, 4:02 PM
kparzysz added inline comments.
test/CodeGen/Hexagon/reg-scavenger-valid-slot.ll
106 ↗(On Diff #57394)

I need to keep enough registers live in a certain range. Turns out that vector register inputs and outputs are not yet implemented for inline-asm. I'll keep the code as is for now.

kparzysz added inline comments.May 17 2016, 4:04 PM
test/CodeGen/Hexagon/reg-scavenger-valid-slot.ll
106 ↗(On Diff #57394)

What I mean is that our backend crashes in isel-lowering on inline-asm with vector inputs/outputs.

kparzysz updated this revision to Diff 57537.May 17 2016, 4:14 PM
kparzysz updated this object.

Added a comment in RegisterScavenging.cpp explaining the motivation for the search loop modification.
Replaced the llvm_unreachable with a report_fatal_error. The message includes register name and register class name.
Applied instnamer to the test, added check lines, and a comment explaining what is being tested.

kparzysz updated this revision to Diff 57627.May 18 2016, 8:13 AM

Created a shorter testcase.

test/CodeGen/Hexagon/reg-scavenger-valid-slot.ll
100 ↗(On Diff #57627)

I have fixed the inline-asm problem.

kparzysz added inline comments.May 18 2016, 8:26 AM
lib/CodeGen/RegisterScavenging.cpp
438–443 ↗(On Diff #57627)

At the second thought---wouldn't it be better to keep the "unreachable"? report_fatal_error does not print the call stack.

qcolombet accepted this revision.May 18 2016, 11:03 AM
qcolombet edited edge metadata.
qcolombet added inline comments.
lib/CodeGen/RegisterScavenging.cpp
414 ↗(On Diff #57627)

What worries me is that we have a quadratic complexity here (#scavengedRegister x Scavenged.size()).
That being said, I guess it is pretty small in practice. Therefore, let us move on and we will adapt if it turns out to be a problem.

438–443 ↗(On Diff #57627)

We could do both maybe.
The problem is that llvm_unreachable expands to nothing in release mode. Thus we may silently generate broken code or a crash somewhere else in the compiler.

This revision is now accepted and ready to land.May 18 2016, 11:03 AM
This revision was automatically updated to reflect the committed changes.
kparzysz added inline comments.May 18 2016, 11:23 AM
llvm/trunk/lib/CodeGen/RegisterScavenging.cpp
445

I kept both.

MatzeB added inline comments.May 18 2016, 4:00 PM
llvm/trunk/lib/CodeGen/RegisterScavenging.cpp
445

This makes no sense. llvm_unreachable() is an indication that this case cannot happen and the compiler will actually optimize for this. There is a high chance it remove the report_fatal_error() in this case.
If you have an error that could actually be provoked by a user of the compiler (through inline assembly or similar) then report_fatal_error() is apropriate. If the problem can only be caused by a bug in llvm then llvm_unreachable() should be used.

kparzysz added inline comments.May 19 2016, 6:15 AM
llvm/trunk/lib/CodeGen/RegisterScavenging.cpp
445

With assertions enabled, llvm_unreachable is the more interesting way to terminate the program, since it will print the call stack. The report_fatal_error will never execute, which is fine. In the release mode, llvm_unreachable will expand to nothing (as per Quentin's earlier remark), but we still want the program to terminate with a meaningful message. It looks strange, but it was intentional.