Page MenuHomePhabricator

[AMDGPU][ScheduleDAG] Check for CopyToReg PhysReg clobbers in pre-RA-sched
ClosedPublic

Authored by jrbyrnes on Jun 27 2022, 1:57 PM.

Details

Summary

This patch includes logic in the Scheduling phase of Instruction selection to check whether or not a CopyToReg will clobber a physical register's live range. If so, the scheduler will not be able to schedule the instruction in the current cycle with the current partial schedule. Without this, the scheduler can schedule CopyToRegs even if they violate a live range, resulting in correctness bugs.

Diff Detail

Event Timeline

jrbyrnes created this revision.Jun 27 2022, 1:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2022, 1:57 PM
jrbyrnes requested review of this revision.Jun 27 2022, 1:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2022, 1:57 PM
jrbyrnes edited the summary of this revision. (Show Details)Jun 27 2022, 1:58 PM
jrbyrnes updated this revision to Diff 440380.Jun 27 2022, 2:02 PM

Ported over to phab review to land in Trunk. Addressed the requests in initial review, renamed test file to better align with naming of previous test.

jrbyrnes retitled this revision from [AMDGPU] Check for CopyToReg PhysReg clobbers in pre-RA-sched to [AMDGPU][ScheduleDAG] Check for CopyToReg PhysReg clobbers in pre-RA-sched.
rampitec added inline comments.Jun 27 2022, 3:59 PM
llvm/lib/CodeGen/SelectionDAG/ScheduleDAGFast.cpp
451

The check for LiveRegDefs[*AI] is excessive. If Node is not null the last check will not succeed.

jrbyrnes updated this revision to Diff 440434.Jun 27 2022, 4:44 PM

Broke up logic in ScheduleDAGFast CheckForLiveRegDef to remove redundancy.

rampitec added inline comments.Jun 28 2022, 12:43 PM
llvm/lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp
1311

Do you really want to insert it if Node is nullptr?

jrbyrnes added inline comments.Jun 28 2022, 4:13 PM
llvm/lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp
1311

Hey Stas -- thanks for comments.

The are two theoretical ways Node will be nullptr. 1. We are using default value, 2. The SDNode we passed is actually a nullptr. Neither way creates a problem for this condition.

  1. All invocations of CheckForLiveRegDef prior to this patch will use the default value. Since this condition is new, in order to maintain the prior behavior, we ignore this condition for the default value.
  1. I assume that this is not possible, and I believe this is a valid assumption. The SDNode we pass is the Node that defines the Src SDValue in CopyToReg. I think the Src for CopyToReg must be coming from SDNode that is not nullptr. I can express this assumption via assert.
rampitec accepted this revision.Jun 28 2022, 4:20 PM

LGTM with a small nit - run instnamer on the test. There should be no unnamed variables. Or rename it manually.

This revision is now accepted and ready to land.Jun 28 2022, 4:20 PM
arsenm added inline comments.Jun 29 2022, 4:53 PM
llvm/test/CodeGen/AMDGPU/copy-to-reg-scc-clobber.ll
2 ↗(On Diff #440434)

If you're going to test the different schedulers, should use the explicit flag for each one

jrbyrnes updated this revision to Diff 441395.Jun 30 2022, 7:41 AM

Run instnamer on testfile, explicitly use "source" (RRList) scheduler for InstSelection Scheduler in test.

jrbyrnes updated this revision to Diff 441396.Jun 30 2022, 7:43 AM

Remove accidental files

arsenm accepted this revision.Jun 30 2022, 7:51 AM
This revision was landed with ongoing or failed builds.Jun 30 2022, 9:18 AM
This revision was automatically updated to reflect the committed changes.