[llvm] r294088 - MachineCopyPropagation: Respect implicit operands of COPY

Hans Wennborg via llvm-commits llvm-commits at lists.llvm.org
Wed Feb 8 08:54:12 PST 2017


Sorry, I forgot you attached a patch. Merged in r294473.

Thanks,
Hans

On Tue, Feb 7, 2017 at 5:46 PM, Matthias Braun <matze at braunis.de> wrote:
> I attached a slightly modified version of the patch to
> http://llvm.org/PR31881 which avoids this problem (you have to consider
> undef operands as read operations unless you have r294087 in place). However
> I consider r294087 a bit too risky without having a few weeks of testing in
> llvm ToT so I would like to keep it out of the 4.0 branch and just go with
> the modified patch as attached to the PR.
>
> - Matthias
>
> On Feb 7, 2017, at 3:33 PM, Hans Wennborg via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
>
> I tried to merge this, but it breaks
> CodeGen/AMDGPU/attr-amdgpu-num-sgpr.ll (see below).
>
> Also merging r294087 seems to fix this.
>
> Matthias: should these be merged together?
>
> Thanks,
> Hans
>
>
> # End machine code for function max_12_sgprs_14_input_sgprs.
>
> *** Bad machine code: Using an undefined physical register ***
> - function:    max_12_sgprs_14_input_sgprs
> - basic block: BB#0  (0x4532e98)
> - instruction: FLAT_STORE_DWORDX2
> - operand 1:   %VGPR0_VGPR1<kill>
> LLVM ERROR: Found 1 machine code errors.
> /usr/local/google/work/llvm-4.0/llvm.src/test/CodeGen/AMDGPU/attr-amdgpu-num-sgpr.ll:45:14:
> error: expected string not found in input
> ; ALL-LABEL: {{^}}max_12_sgprs_14_input_sgprs:
>             ^
> <stdin>:13:15: note: scanning from here
> max_12_sgprs: ; @max_12_sgprs
>              ^
>
> --
>
>
>
>
> On Tue, Feb 7, 2017 at 1:42 PM, Quentin Colombet <qcolombet at apple.com>
> wrote:
>
> Hi,
>
> SGTM (I’ve already reviewed the phab)
>
> Cheers,
> -Quentin
>
> On Feb 7, 2017, at 9:14 AM, Hans Wennborg <hans at chromium.org> wrote:
>
> Quentin: Matthias has asked for this to be merged to 4.0 (see
> PR31881). What do you think?
>
> Thanks,
> Hans
>
> On Fri, Feb 3, 2017 at 6:27 PM, Matthias Braun via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
>
> Author: matze
> Date: Fri Feb  3 20:27:20 2017
> New Revision: 294088
>
> URL: http://llvm.org/viewvc/llvm-project?rev=294088&view=rev
> Log:
> MachineCopyPropagation: Respect implicit operands of COPY
>
> The code missed to check implicit operands of COPY instructions for
> defs/uses.
>
> Differential Revision: https://reviews.llvm.org/D29522
>
> Added:
>   llvm/trunk/test/CodeGen/ARM/machine-copyprop.mir
> Modified:
>   llvm/trunk/lib/CodeGen/MachineCopyPropagation.cpp
>
> Modified: llvm/trunk/lib/CodeGen/MachineCopyPropagation.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineCopyPropagation.cpp?rev=294088&r1=294087&r2=294088&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/MachineCopyPropagation.cpp (original)
> +++ llvm/trunk/lib/CodeGen/MachineCopyPropagation.cpp Fri Feb  3 20:27:20
> 2017
> @@ -61,6 +61,7 @@ namespace {
>
>  private:
>    void ClobberRegister(unsigned Reg);
> +    void ReadRegister(unsigned Reg);
>    void CopyPropagateBlock(MachineBasicBlock &MBB);
>    bool eraseIfRedundant(MachineInstr &Copy, unsigned Src, unsigned Def);
>
> @@ -120,6 +121,18 @@ void MachineCopyPropagation::ClobberRegi
>  }
> }
>
> +void MachineCopyPropagation::ReadRegister(unsigned Reg) {
> +  // If 'Reg' is defined by a copy, the copy is no longer a candidate
> +  // for elimination.
> +  for (MCRegAliasIterator AI(Reg, TRI, true); AI.isValid(); ++AI) {
> +    Reg2MIMap::iterator CI = CopyMap.find(*AI);
> +    if (CI != CopyMap.end()) {
> +      DEBUG(dbgs() << "MCP: Copy is used - not dead: ";
> CI->second->dump());
> +      MaybeDeadCopies.remove(CI->second);
> +    }
> +  }
> +}
> +
> /// Return true if \p PreviousCopy did copy register \p Src to register \p
> Def.
> /// This fact may have been obscured by sub register usage or may not be
> true at
> /// all even though Src and Def are subregisters of the registers used in
> @@ -212,12 +225,14 @@ void MachineCopyPropagation::CopyPropaga
>
>      // If Src is defined by a previous copy, the previous copy cannot be
>      // eliminated.
> -      for (MCRegAliasIterator AI(Src, TRI, true); AI.isValid(); ++AI) {
> -        Reg2MIMap::iterator CI = CopyMap.find(*AI);
> -        if (CI != CopyMap.end()) {
> -          DEBUG(dbgs() << "MCP: Copy is no longer dead: ";
> CI->second->dump());
> -          MaybeDeadCopies.remove(CI->second);
> -        }
> +      ReadRegister(Src);
> +      for (const MachineOperand &MO : MI->implicit_operands()) {
> +        if (!MO.isReg() || !MO.readsReg())
> +          continue;
> +        unsigned Reg = MO.getReg();
> +        if (!Reg)
> +          continue;
> +        ReadRegister(Reg);
>      }
>
>      DEBUG(dbgs() << "MCP: Copy is a deletion candidate: "; MI->dump());
> @@ -234,6 +249,14 @@ void MachineCopyPropagation::CopyPropaga
>      // ...
>      // %xmm2<def> = copy %xmm9
>      ClobberRegister(Def);
> +      for (const MachineOperand &MO : MI->implicit_operands()) {
> +        if (!MO.isReg() || !MO.isDef())
> +          continue;
> +        unsigned Reg = MO.getReg();
> +        if (!Reg)
> +          continue;
> +        ClobberRegister(Reg);
> +      }
>
>      // Remember Def is defined by the copy.
>      for (MCSubRegIterator SR(Def, TRI, /*IncludeSelf=*/true); SR.isValid();
> @@ -269,17 +292,8 @@ void MachineCopyPropagation::CopyPropaga
>      if (MO.isDef()) {
>        Defs.push_back(Reg);
>        continue;
> -      }
> -
> -      // If 'Reg' is defined by a copy, the copy is no longer a candidate
> -      // for elimination.
> -      for (MCRegAliasIterator AI(Reg, TRI, true); AI.isValid(); ++AI) {
> -        Reg2MIMap::iterator CI = CopyMap.find(*AI);
> -        if (CI != CopyMap.end()) {
> -          DEBUG(dbgs() << "MCP: Copy is used - not dead: ";
> CI->second->dump());
> -          MaybeDeadCopies.remove(CI->second);
> -        }
> -      }
> +      } else if (MO.readsReg())
> +        ReadRegister(Reg);
>    }
>
>    // The instruction has a register mask operand which means that it
> clobbers
>
> Added: llvm/trunk/test/CodeGen/ARM/machine-copyprop.mir
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/ARM/machine-copyprop.mir?rev=294088&view=auto
> ==============================================================================
> --- llvm/trunk/test/CodeGen/ARM/machine-copyprop.mir (added)
> +++ llvm/trunk/test/CodeGen/ARM/machine-copyprop.mir Fri Feb  3 20:27:20
> 2017
> @@ -0,0 +1,22 @@
> +# RUN: llc -o - %s -mtriple=armv7s-- -run-pass=machine-cp | FileCheck %s
> +---
> +# Test that machine copy prop recognizes the implicit-def operands on a
> COPY
> +# as clobbering the register.
> +# CHECK-LABEL: name: func
> +# CHECK: %d2 = VMOVv2i32 2, 14, _
> +# CHECK: %s5 = COPY %s0, implicit %q1, implicit-def %q1
> +# CHECK: VST1q32 %r0, 0, %q1, 14, _
> +# The following two COPYs must not be removed
> +# CHECK: %s4 = COPY %s20, implicit-def %q1
> +# CHECK: %s5 = COPY %s0, implicit killed %d0, implicit %q1, implicit-def
> %q1
> +# CHECK: VST1q32 %r2, 0, %q1, 14, _
> +name: func
> +body: |
> +  bb.0:
> +    %d2 = VMOVv2i32 2, 14, _
> +    %s5 = COPY %s0, implicit %q1, implicit-def %q1
> +    VST1q32 %r0, 0, %q1, 14, _
> +    %s4 = COPY %s20, implicit-def %q1
> +    %s5 = COPY %s0, implicit killed %d0, implicit %q1, implicit-def %q1
> +    VST1q32 %r2, 0, %q1, 14, _
> +...
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>


More information about the llvm-commits mailing list