Page MenuHomePhabricator

[SystemZ] Mark unsaved argument R6 as live throughout function
ClosedPublic

Authored by jonpa on Oct 15 2020, 1:11 AM.

Details

Summary

For historical reasons, the R6 register is a callee saved argument register. This means that if it is used to pass an argument to a function that does not clobber it, it is live throughout the function.

This patch makes sure that in this special case any kill flags of it are removed, all MBBs have it in their live-in lists and all return instructions use it.

Diff Detail

Event Timeline

jonpa created this revision.Oct 15 2020, 1:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2020, 1:11 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
jonpa requested review of this revision.Oct 15 2020, 1:11 AM

This seems like the right approach. It feels weird, but I can't think of anything better.

llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
356

Not sure I understand what the LowGPR && SystemZMC::getFirstReg(LowGPR) > SystemZMC::getFirstReg(SystemZ::R6D) is doing. We should be checking whether the register was spilled in the prologue.

llvm/test/CodeGen/SystemZ/frame-26.ll
6 ↗(On Diff #298324)

Probably want an MIR testcase; smaller, and more likely to continue to hit the case in question in the future.

uweigand added inline comments.Oct 16 2020, 1:57 AM
llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
356

That's exactly what this check does: we use a STORE MULTIPLE instruction to spill the range of registers from LowGPR up to r15. So "LowGPR > r6" is equivalent to "r6 is not spilled in the prologue".

jonpa updated this revision to Diff 298575.Oct 16 2020, 3:31 AM
jonpa marked 2 inline comments as done.

Updated per review to use a MIR test instead.

I cleaned it up as best I could, but I could not get rid of the IsSSA flag being set, but it seems generally ignored in tests.. For instance AArch64/wineh-frame3.mir also has this problem. It doesn't seem to matter though...

bin/llc -o - ../llvm/test/CodeGen/AArch64/wineh-frame3.mir -mtriple=aarch64-windows -start-before=prologepilog -print-after=prologepilog                                                                                                      
        .text                                                                                                          
        .file   "wineh-frame3.mir"                                                                                     
# *** IR Dump After Prologue/Epilogue Insertion & Frame Finalization ***:                                              
# Machine code for function test: IsSSA, NoPHIs, TracksLiveness, NoVRegs
This revision is now accepted and ready to land.Oct 16 2020, 2:47 PM
uweigand added inline comments.Oct 16 2020, 5:30 PM
llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
369

Just one question here: do we need to explicitly add this use? If I followed your description in the bugzilla correctly, all callee-saved register are implicitly considered used at the end of the return block?

jonpa added inline comments.Oct 17 2020, 3:06 AM
llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
369

Good question... It seems both LiveRegUnits and LivePhysRegs classes do special-treat the return blocks so it might be that this is implicitly modelled in the IR. RegScavenger uses LiveRegUnits (I don't know why there are two different classes for this) so in this particular case the use operands are not needed.

I now see a comment at LivePhysRegs::addLiveOutsNoPristines():

// Return blocks are a special case because we currently don't mark up
// return instructions completely: specifically, there is no explicit
// use for callee-saved registers. So we add all callee saved registers
// that are saved and restored (somewhere). This does not include
// callee saved registers that are unused and hence not saved and
// restored; they are called pristine.
// FIXME: PEI should add explicit markings to return instructions
// instead of implicitly handling them here.

It seems there is some intent on moving towards explicit use operands on return instructions instead?

The notion of a "pristine" register meaning an unused callee saved reg, makes me think we may want to be explicit about R6 since that seems to imply it is completely unused, which may not be true...?

efriedma added inline comments.Oct 17 2020, 11:44 AM
llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
369

I guess LivePhysRegs/LiveRegUnits will treat r6 as "pristine" since we don't save/restore it. I guess, thinking about it a bit more, that might means it's okay to skip marking up the liveins and the return instructions; it's implicitly live anyway.

As far as I can tell, nothing cares if something reads a pristine register; it's only a problem if an instruction writes/kills one. But there's so little code that runs this late in the pipeline, I'm not sure how consistent everything really is, anyway.

jonpa updated this revision to Diff 299640.Oct 21 2020, 4:47 AM
  • Removed the adding of R6 to live-in lists and return instructions.
  • Changed the check to use getRestoreGPRRegs() instead of getSpillGPRRegs(). These may differ for a vararg function, where a vararg R6 is spilled but not restored. In that case it is still unclobbered and live throughout the function. (The spilling of vararg r6 will first be marked with kill, but this patch will remove it).
  • Changed the check to compare against SystemZ::R6 directly since that should be the lowest restored GPR.
jonpa requested review of this revision.Oct 21 2020, 4:47 AM
uweigand accepted this revision.Oct 21 2020, 5:18 AM

See inline comment; otherwise this LGTM.

llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
357

Just a minor nit, maybe the loop can be written more compact using C++11 features like:

for (auto &I : MRI->use_nodbg_operands(SystemZ::R6D))

This should be equivalent as far as I can see.

This revision is now accepted and ready to land.Oct 21 2020, 5:18 AM
This revision was automatically updated to reflect the committed changes.
jonpa marked an inline comment as done.