[llvm] r294088 - MachineCopyPropagation: Respect implicit operands of COPY
Matthias Braun via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 7 17:46:24 PST 2017
I attached a slightly modified version of the patch to http://llvm.org/PR31881 <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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170207/21a83230/attachment.html>
More information about the llvm-commits
mailing list