[NFC] [TargetRegisterInfo] add one use check to lookThruCopyLike.

[shchenz]

Summary

add one use check to lookThruCopyLike.

The root node is safe to be deleted if we are sure that every definition in the copy chain only has one use.

https://reviews.llvm.org/D92071 is a user of this modified function.

[shchenz]

lint warning fix

[shchenz]

gentle ping...

[shchenz]

gentle ping

[shchenz]

gentle ping

[jsji]

LGTM.

[echristo]

Sorry to point this out now, but taking a bool in/out pointer is some
fairly awkward API. Can we do something else here?

Thanks!

-eric

[shchenz]

In D92069#2503990, @echristo wrote:

Sorry to point this out now, but taking a bool in/out pointer is some
fairly awkward API. Can we do something else here?

Thanks!

-eric

Yes, I am happy to change this to another version. Do you think returning std::pair<Register, bool> is an acceptable way?

[echristo]

I'd like to avoid the bool completely if possible. Passing the register
might work as well if you wanted to do that, but I think it needs to be
clear API wise why and how you're doing computation where you are.

Do you have the follow on patch?

-eric

[shchenz]

In D92069#2504002, @echristo wrote:

I'd like to avoid the bool completely if possible. Passing the register
might work as well if you wanted to do that, but I think it needs to be
clear API wise why and how you're doing computation where you are.

Do you have the follow on patch?

-eric

I don't have the follow on yet. Sorry, I am not very clear about the passing the register method to avoid the bool type. Could you please give me more details? Thanks.

User patch of this API is in patch D92071

[jsji]

In D92069#2504002, @echristo wrote:

I'd like to avoid the bool completely if possible.

Thanks @echristo for bring up this. We are happy to change it for better code.

However, can you help us to understand the reason, and any advice about how to design this API?
Also is this PowerPC specific or general to LLVM coding style?

As we are seeing quite decent similar usages in the code base,
eg:

$ grep "bool \*" llvm/include/llvm -R |wc
    53     332    5185

If this is general, do you think it worth mentioning in ProgrammersManual?
If so, we can follow up a patch to update that as well.

[nemanjai]

I don't see an issue in providing two functions to achieve the two different goals:

lookThruCopyLike()           // Look through a chain of copies.
lookThruSingleUseCopyChain() // Look through a chain of copies, checking that each def has a single use.

Also, I am a little curious about what this change is meant to accomplish. The name of the introduced bool parameter is AllDefHaveOneUser yet we don't actually check all defs - we only check the final one that we are returning. So it is presumably possible to have something like this (all regs virtual):

%1 = ...
%2 = SUBREG_TO_REG %1, ...
%3 = COPY %2
%4 = SUBREG_TO_REG %2
%5 = COPY %4
%6 = COPY %4
%7 = <some use of %4>
%8 = <some use of %5>
%9 = <some use of %6>

Running lookThruCopyLike() on %9 will presumably correctly return %1 and if a bool argument is passed, it will be set to true. I would argue that this is a surprising result since I would expect AllDefHaveOneUser to mean that all the definitions traversed (%9, %6, %4, %2, %1) have a single use. But this is clearly not the case. Am I just missing something?

[shchenz]

address comments

[shchenz]

@nemanjai Hi, thanks for your comments. You are right. The first version is not what I intend to implement. I have updated the patch according to your comments.

The original revision was reverted. Could you please help to do another review? Thanks. @echristo @nemanjai @jsji

[nemanjai]

LGTM now. Maybe give @echristo a bit of time to see if this adequately addresses his original concern.

llvm/lib/CodeGen/TargetRegisterInfo.cpp

541–547

Minor nit: Stylistically, it is probably a bit neater to write this as:

// Found the real definition, return it if it has a single use.
if (!MI->isCopyLike())
  return MRI->hasOneNonDBGUse(SrcReg) ? SrcReg : Register();

557

Maybe it is clearer as:

// Continue only if the next definition in the chain is for a virtual
// register that has a single use.

[shchenz]

1: address @nemanjai comments

[echristo]

llvm/lib/CodeGen/TargetRegisterInfo.cpp

536

You should document what it's doing ahead of time as well. It's not clear when and how you'd use this function.

[shchenz]

llvm/lib/CodeGen/TargetRegisterInfo.cpp

536

Thank you, @echristo . Do you think is it ok to add

This function is called when try to delete the root non-copy virtual register. We must make sure the root non-copy virtual register and all the copy like definitions in the chain have only one user.

In the header?

llvm/include/llvm/CodeGen/TargetRegisterInfo.h

public:
 
   /// Returns the original SrcReg unless it is the target of a copy-like
   /// operation, in which case we chain backwards through all such operations
   /// to the ultimate source register.  If a physical register is encountered,
   /// we stop the search.
   virtual Register lookThruCopyLike(Register SrcReg,
                                     const MachineRegisterInfo *MRI) const;
 
+  /// Find the original SrcReg unless it is the target of a copy-like operation,
+  /// in which case we chain backwards through all such operations to the
+  /// ultimate source register. If a physical register is encountered, we stop
+  /// the search.
+  /// Return the original SrcReg if all the definitions in the chain only have
+  /// one user and not a physical register.
+  virtual Register
+  lookThruSingleUseCopyChain(Register SrcReg,
+                             const MachineRegisterInfo *MRI) const;
+
   /// Return a null-terminated list of all of the callee-saved registers on
   /// this target. The register should be in the order of desired callee-save
   /// stack frame offset. The first register is closest to the incoming stack
   /// pointer if stack grows down, and vice versa.
   /// Notice: This function does not take into account disabled CSRs.
   ///         In most cases you will want to use instead the function
   ///         getCalleeSavedRegs that is implemented in MachineRegisterInfo.
   virtual const MCPhysReg*

llvm/lib/CodeGen/TargetRegisterInfo.cpp

while (true) {
 
     if (!CopySrcReg.isVirtual())
       return CopySrcReg;
 
     SrcReg = CopySrcReg;
   }
 }
 
+Register TargetRegisterInfo::lookThruSingleUseCopyChain(
+    Register SrcReg, const MachineRegisterInfo *MRI) const {
+  while (true) {
+    const MachineInstr *MI = MRI->getVRegDef(SrcReg);
+    // Found the real definition, return it if it has a single use.
+    if (!MI->isCopyLike())
+      return MRI->hasOneNonDBGUse(SrcReg) ? SrcReg : Register();
+
+    Register CopySrcReg;
+    if (MI->isCopy())
+      CopySrcReg = MI->getOperand(1).getReg();
+    else {
+      assert(MI->isSubregToReg() && "Bad opcode for lookThruCopyLike");
+      CopySrcReg = MI->getOperand(2).getReg();
+    }
+
+    // Continue only if the next definition in the chain is for a virtual
+    // register that has a single use.
+    if (!CopySrcReg.isVirtual() || !MRI->hasOneNonDBGUse(CopySrcReg))
+      return Register();
+
+    SrcReg = CopySrcReg;
+  }
+}
+
 void TargetRegisterInfo::getOffsetOpcodes(
     const StackOffset &Offset, SmallVectorImpl<uint64_t> &Ops) const {
   assert(!Offset.getScalable() && "Scalable offsets are not handled");
   DIExpression::appendOffset(Ops, Offset.getFixed());
 }
 
 DIExpression *
 TargetRegisterInfo::prependOffsetExpression(const DIExpression *Expr,