[PATCH] D139679: [X86] Don't zero out %eax if both %al and %ah are used

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers accepted this revision.
nickdesaulniers added a comment.
This revision is now accepted and ready to land.

I think x86 is an oddity in that `TargetRegisterInfo::sub_and_superregs_inclusive` doesn't return sub_8bit or sub_8bit_hi. I suspect this is because in llvm/lib/Target/X86/X86RegisterInfo.td they're not marked as `CoveredBySubRegs`.  This patch isn't doing that much work; it's looking at each MachineBasicBlock terminator operands if the terminate is a return instruction only.

The offending MachineInstr in this case is `RET 0, $al`.

I spent some time playing with MCRegisterDesc and TargetRegisterInfo::getNumSubRegIndices (and sub register indices in general) and couldn't quite get what I wanted.  `getX86SubSuperRegister` looks like a hack/lacks symmetry/works around `CoveredBySubRegs` and I can't help but wonder if that's a bug, or if marking sub_8bit or sub_8bit_hi as CoveredBySubRegs would break other things.

Please update the comment and add nounwind to the test.



================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:527
 
+  // If an 8-bit register is live out, we don't want to clear either the high
+  // or low version of the register.
----------------
void wrote:
> nickdesaulniers wrote:
> > Is live out of the function itself? Or between basic blocks?
> > 
> > MachineRegisterInfo can answer whether a given register is live in `MachineRegisterInfo::isLiveIn`. I assume that's the same side of the coin for "is a given register live out of my predecessor?"  Perhaps that's better to query than looking into every operand of every instruction in the function?
> > 
> > MachineBasicBlock::liveouts() also gives you an iterator to a RegisterPairMask. Might be useful here?
> MachineBasicBlock::liveouts() doesn't appear to work here.
Please update the comment to s/live out/live out of the function's return instructions/


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139679/new/

https://reviews.llvm.org/D139679