This is an archive of the discontinued LLVM Phabricator instance.

Differential D106312

[AMDGPU] Allow rematerialization of virtual reg uses
AbandonedPublic

Authored by rampitec on Jul 19 2021, 2:10 PM.

Details

Reviewers
arsenm
Summary

Currently isReallyTriviallyReMaterializableGeneric() implementation
prevents rematerialization on any virtual register use on the grounds
that is not a trivial rematerialization and that we do not want to
extend liveranges.

It appears that LRE logic does not attempt to extend a liverange of
a source register for rematerialization so that is not an issue.
That is checked in the LiveRangeEdit::allUsesAvailableAt().

The only non-trivial aspect of it on top of what we are already
checking for VOP (e.g. implicit uses/defs) and which is already working
with our SIInstrInfo::isReallyTriviallyReMaterializable() is accounting
for tied-defs which normally represent a read-modify-write operation
and not rematerializable. Everything else is proven to work by the
existing VOP implementation. We already allow such uses and bypassing
the rest of generic checks for VOP, but cannot rematerialize SOP
instructions with register uses.

The test for a tied-def situation already exists in the remat-vop.mir,
test_no_remat_v_cvt_f32_i32_sdwa_dst_unused_preserve.

Applying the same logic to other targets shows that there are really
non-trivial situations with uses so the logic is not generally
acceptable for any target.

Diff Detail

Unit TestsFailed

TimeTest
2,630 msx64 debian > libarcher.critical::critical.c
2,750 msx64 debian > libarcher.parallel::parallel-firstprivate.c
2,780 msx64 debian > libarcher.races::critical-unrelated.c
2,710 msx64 debian > libarcher.races::lock-nested-unrelated.c
2,970 msx64 debian > libarcher.races::lock-unrelated.c
View Full Test Results (17 Failed)

Event Timeline

rampitec created this revision.Jul 19 2021, 2:10 PM
Herald added subscribers: foad, kerbowa, hiraditya and 7 others. · View Herald TranscriptJul 19 2021, 2:10 PM
rampitec requested review of this revision.Jul 19 2021, 2:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2021, 2:10 PM
Herald added a subscriber: wdng. · View Herald Transcript
Harbormaster completed remote builds in B114944: Diff 359910.Jul 19 2021, 3:16 PM
arsenm added a comment.Jul 19 2021, 3:40 PM

I don't follow this. Why would we want to override the generic logic and extend live ranges? If the LRE checks that it doesn't extend the live range, why not just remove the generic check?

rampitec added a comment.Jul 19 2021, 3:48 PM
In D106312#2888778, @arsenm wrote:

I don't follow this. Why would we want to override the generic logic and extend live ranges? If the LRE checks that it doesn't extend the live range, why not just remove the generic check?

It does not extend live ranges, I have added a test for it. The comment is likely just obsolete. But then if I simply remove the check from the generic implementation a lot of tests in other targets start to fail. These must have their own restrictions which do not apply to us. For instance our restriction will be to avoid tied-defs and would have to go into generic logic then.

arsenm added a comment.Jul 19 2021, 3:57 PM
In D106312#2888802, @rampitec wrote:
In D106312#2888778, @arsenm wrote:

I don't follow this. Why would we want to override the generic logic and extend live ranges? If the LRE checks that it doesn't extend the live range, why not just remove the generic check?

It does not extend live ranges, I have added a test for it. The comment is likely just obsolete. But then if I simply remove the check from the generic implementation a lot of tests in other targets start to fail. These must have their own restrictions which do not apply to us.

This could also just mean code improvements. You can't just assume these are incorrect changes without looking. I don't see much special going on in the other target implementations

rampitec added a comment.Jul 19 2021, 3:59 PM
In D106312#2888820, @arsenm wrote:
In D106312#2888802, @rampitec wrote:
In D106312#2888778, @arsenm wrote:

I don't follow this. Why would we want to override the generic logic and extend live ranges? If the LRE checks that it doesn't extend the live range, why not just remove the generic check?

It does not extend live ranges, I have added a test for it. The comment is likely just obsolete. But then if I simply remove the check from the generic implementation a lot of tests in other targets start to fail. These must have their own restrictions which do not apply to us.

This could also just mean code improvements. You can't just assume these are incorrect changes without looking. I don't see much special going on in the other target implementations

Using vreg without a def is not an improvement, just as other sorts of failed verification.

arsenm added a comment.Jul 19 2021, 5:20 PM
In D106312#2888821, @rampitec wrote:
In D106312#2888820, @arsenm wrote:
In D106312#2888802, @rampitec wrote:
In D106312#2888778, @arsenm wrote:

I don't follow this. Why would we want to override the generic logic and extend live ranges? If the LRE checks that it doesn't extend the live range, why not just remove the generic check?

It does not extend live ranges, I have added a test for it. The comment is likely just obsolete. But then if I simply remove the check from the generic implementation a lot of tests in other targets start to fail. These must have their own restrictions which do not apply to us.

This could also just mean code improvements. You can't just assume these are incorrect changes without looking. I don't see much special going on in the other target implementations

Using vreg without a def is not an improvement, just as other sorts of failed verification.

This could also mean we just don't have a test that hits this. I think it would be worth looking into these failures to see if this really is conceptually correct

rampitec added a comment.Jul 19 2021, 5:22 PM
In D106312#2888993, @arsenm wrote:
In D106312#2888821, @rampitec wrote:
In D106312#2888820, @arsenm wrote:
In D106312#2888802, @rampitec wrote:
In D106312#2888778, @arsenm wrote:

I don't follow this. Why would we want to override the generic logic and extend live ranges? If the LRE checks that it doesn't extend the live range, why not just remove the generic check?

It does not extend live ranges, I have added a test for it. The comment is likely just obsolete. But then if I simply remove the check from the generic implementation a lot of tests in other targets start to fail. These must have their own restrictions which do not apply to us.

This could also just mean code improvements. You can't just assume these are incorrect changes without looking. I don't see much special going on in the other target implementations

Using vreg without a def is not an improvement, just as other sorts of failed verification.

This could also mean we just don't have a test that hits this. I think it would be worth looking into these failures to see if this really is conceptually correct

I just have to note, we are doing this for ages. Just for select valu moves.

rampitec added a comment.Jul 20 2021, 12:31 PM

OK, here is the test which fails just the same way as aarch64 without any changes at all:

# RUN: llc -march=amdgcn -mcpu=gfx900 -o - -verify-coalescing -run-pass=simple-register-coalescing %s

---
name:            test
tracksRegLiveness: true
body:             |
  bb.0:
    liveins: $vgpr0

    %0:vgpr_32 = COPY $vgpr0
    %1:vgpr_32 = V_ADD_U32_e32 1, %0, implicit $exec
    %2:vgpr_32 = V_MOV_B32_e32 killed %1, implicit $exec
    $vgpr0 = COPY killed %2
    SI_RETURN_TO_EPILOG killed $vgpr0
...

We allow rematerialization of the V_MOV_B32 into the COPY, but %1 is killed at that slot and it ends up as:

0B      bb.0:
          liveins: $vgpr0
16B       KILL $vgpr0
64B       $vgpr0 = V_MOV_B32_e32 %1:vgpr_32, implicit $exec
80B       SI_RETURN_TO_EPILOG $vgpr0
rampitec added a comment.Jul 20 2021, 12:34 PM

Another form of the same test, without a killed use but with a later separate KILL:

# RUN: llc -march=amdgcn -mcpu=gfx900 -o - -verify-coalescing -run-pass=simple-register-coalescing %s

---
name:            test
tracksRegLiveness: true
body:             |
  bb.0:
    liveins: $vgpr0

    %0:vgpr_32 = COPY $vgpr0
    %1:vgpr_32 = V_ADD_U32_e32 1, %0, implicit $exec
    %2:vgpr_32 = V_MOV_B32_e32 %1, implicit $exec
    KILL %1
    $vgpr0 = COPY killed %2
    SI_RETURN_TO_EPILOG killed $vgpr0
...

Looks like LRE checks that all uses are available, but coalescer does not.

rampitec added a comment.Jul 20 2021, 2:00 PM
In D106312#2891346, @rampitec wrote:

Another form of the same test, without a killed use but with a later separate KILL:

# RUN: llc -march=amdgcn -mcpu=gfx900 -o - -verify-coalescing -run-pass=simple-register-coalescing %s

---
name:            test
tracksRegLiveness: true
body:             |
  bb.0:
    liveins: $vgpr0

    %0:vgpr_32 = COPY $vgpr0
    %1:vgpr_32 = V_ADD_U32_e32 1, %0, implicit $exec
    %2:vgpr_32 = V_MOV_B32_e32 %1, implicit $exec
    KILL %1
    $vgpr0 = COPY killed %2
    SI_RETURN_TO_EPILOG killed $vgpr0
...

Looks like LRE checks that all uses are available, but coalescer does not.

Should be fixed by D106396.

rampitec added a comment.Jul 20 2021, 3:59 PM

Alternative D106408 does that for all targets.

rampitec abandoned this revision.Aug 16 2021, 12:07 PM

Revision Contents

PathSize
llvm/
lib/
Target/
AMDGPU/
26 lines
test/
CodeGen/
AMDGPU/
60 lines

Diff 359910

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 101 Lines • ▼ Show 20 Linesstatic bool nodesHaveSameOperandValue(SDNode *N0, SDNode* N1, unsigned OpName) {
--Op0Idx; --Op0Idx;
--Op1Idx; --Op1Idx;
return N0->getOperand(Op0Idx) == N1->getOperand(Op1Idx); return N0->getOperand(Op0Idx) == N1->getOperand(Op1Idx);
} }
bool SIInstrInfo::isReallyTriviallyReMaterializable(const MachineInstr &MI, bool SIInstrInfo::isReallyTriviallyReMaterializable(const MachineInstr &MI,
AAResults *AA) const { AAResults *AA) const {
if (isVOP1(MI) || isVOP3(MI) || isSDWA(MI)) {
// Normally VALU use of exec would block the rematerialization, but that // Normally VALU use of exec would block the rematerialization, but that
// is OK in this case to have an implicit exec read as all VALU do. // is OK in this case to have an implicit exec read as all VALU do.
// We really want all of the generic logic for this except for this.
// Another potential implicit use is mode register. The core logic of // Another potential implicit use is mode register. The core logic of
// the RA will not attempt rematerialization if mode is set anywhere // the RA will not attempt rematerialization if mode is set anywhere
// in the function, otherwise it is safe since mode is not changed. // in the function, otherwise it is safe since mode is not changed.
return !MI.hasImplicitDef() &&
MI.getNumImplicitOperands() == MI.getDesc().getNumImplicitUses();
}
return false; // A generic implementation bails on only virtual register use which
// effectively prevents rematerialization. On AMDGPU we only need to
// check we have no tied operands which normally model read-modify-write.
return !MI.hasImplicitDef() &&
MI.getNumImplicitOperands() == MI.getDesc().getNumImplicitUses() &&
!MI.getOperand(0).isTied();
} }
bool SIInstrInfo::isIgnorableUse(const MachineOperand &MO) const { bool SIInstrInfo::isIgnorableUse(const MachineOperand &MO) const {
// Any implicit use of exec by VALU is not a real register read. // Any implicit use of exec by VALU is not a real register read.
return MO.getReg() == AMDGPU::EXEC && MO.isImplicit() && return MO.getReg() == AMDGPU::EXEC && MO.isImplicit() &&
isVALU(*MO.getParent()); isVALU(*MO.getParent());
} }
▲ Show 20 Lines • Show All 7,778 Lines • Show Last 20 Lines

llvm/test/CodeGen/AMDGPU/remat-sop.mir

Show First 20 Lines • Show All 45 Lines • ▼ Show 20 Lines bb.0:
%0:sreg_32 = S_MOV_B32 1, implicit $exec %0:sreg_32 = S_MOV_B32 1, implicit $exec
%1:sreg_32 = S_MOV_B32 2, implicit $exec %1:sreg_32 = S_MOV_B32 2, implicit $exec
%2:sreg_32 = S_MOV_B32 3, implicit $exec %2:sreg_32 = S_MOV_B32 3, implicit $exec
S_NOP 0, implicit %0 S_NOP 0, implicit %0
S_NOP 0, implicit %1 S_NOP 0, implicit %1
S_NOP 0, implicit %2 S_NOP 0, implicit %2
S_ENDPGM 0 S_ENDPGM 0
... ...
# The liverange of %0 covers a point of rematerialization, source value is
# availabe.
---
name: test_remat_s_mov_b32_vreg_src_long_lr
tracksRegLiveness: true
machineFunctionInfo:
stackPtrOffsetReg: $sgpr32
body: |
bb.0:
; GCN-LABEL: name: test_remat_s_mov_b32_vreg_src_long_lr
; GCN: renamable $sgpr0 = IMPLICIT_DEF
; GCN: renamable $sgpr1 = S_MOV_B32 renamable $sgpr0
; GCN: S_NOP 0, implicit killed renamable $sgpr1
; GCN: renamable $sgpr1 = S_MOV_B32 renamable $sgpr0
; GCN: S_NOP 0, implicit killed renamable $sgpr1
; GCN: renamable $sgpr1 = S_MOV_B32 renamable $sgpr0
; GCN: S_NOP 0, implicit killed renamable $sgpr1
; GCN: S_NOP 0, implicit killed renamable $sgpr0
; GCN: S_ENDPGM 0
%0:sreg_32 = IMPLICIT_DEF
%1:sreg_32 = S_MOV_B32 %0:sreg_32
%2:sreg_32 = S_MOV_B32 %0:sreg_32
%3:sreg_32 = S_MOV_B32 %0:sreg_32
S_NOP 0, implicit %1
S_NOP 0, implicit %2
S_NOP 0, implicit %3
S_NOP 0, implicit %0
S_ENDPGM 0
...
# The liverange of %0 does not cover a point of rematerialization, source value is
# unavailabe and we do not want to artificially extend the liverange.
---
name: test_no_remat_s_mov_b32_vreg_src_short_lr
tracksRegLiveness: true
machineFunctionInfo:
stackPtrOffsetReg: $sgpr32
body: |
bb.0:
; GCN-LABEL: name: test_no_remat_s_mov_b32_vreg_src_short_lr
; GCN: renamable $sgpr0 = IMPLICIT_DEF
; GCN: renamable $sgpr1 = S_MOV_B32 renamable $sgpr0
; GCN: SI_SPILL_S32_SAVE killed renamable $sgpr1, %stack.1, implicit $exec, implicit $sgpr32 :: (store (s32) into %stack.1, addrspace 5)
; GCN: renamable $sgpr1 = S_MOV_B32 renamable $sgpr0
; GCN: SI_SPILL_S32_SAVE killed renamable $sgpr1, %stack.0, implicit $exec, implicit $sgpr32 :: (store (s32) into %stack.0, addrspace 5)
; GCN: renamable $sgpr0 = S_MOV_B32 killed renamable $sgpr0
; GCN: renamable $sgpr1 = SI_SPILL_S32_RESTORE %stack.1, implicit $exec, implicit $sgpr32 :: (load (s32) from %stack.1, addrspace 5)
; GCN: S_NOP 0, implicit killed renamable $sgpr1
; GCN: renamable $sgpr1 = SI_SPILL_S32_RESTORE %stack.0, implicit $exec, implicit $sgpr32 :: (load (s32) from %stack.0, addrspace 5)
; GCN: S_NOP 0, implicit killed renamable $sgpr1
; GCN: S_NOP 0, implicit killed renamable $sgpr0
; GCN: S_ENDPGM 0
%0:sreg_32 = IMPLICIT_DEF
%1:sreg_32 = S_MOV_B32 %0:sreg_32
%2:sreg_32 = S_MOV_B32 %0:sreg_32
%3:sreg_32 = S_MOV_B32 %0:sreg_32
S_NOP 0, implicit %1
S_NOP 0, implicit %2
S_NOP 0, implicit %3
S_ENDPGM 0
...
--- ---
name: test_remat_s_mov_b64 name: test_remat_s_mov_b64
tracksRegLiveness: true tracksRegLiveness: true
body: | body: |
bb.0: bb.0:
; GCN-LABEL: name: test_remat_s_mov_b64 ; GCN-LABEL: name: test_remat_s_mov_b64
; GCN: renamable $sgpr0_sgpr1 = S_MOV_B64 1 ; GCN: renamable $sgpr0_sgpr1 = S_MOV_B64 1
; GCN: renamable $sgpr2_sgpr3 = S_MOV_B64 2 ; GCN: renamable $sgpr2_sgpr3 = S_MOV_B64 2
▲ Show 20 Lines • Show All 454 Lines • Show Last 20 Lines