This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU/SI: Enable the post-ra scheduler
ClosedPublic

Authored by tstellarAMD on Mar 30 2016, 8:23 AM.

Details

Summary

This includes a hazard recognizer implementation to replace some of
the hazard handling we had during frame index elimination.

Diff Detail

Repository
rL LLVM

Event Timeline

tstellarAMD retitled this revision from to AMDGPU/SI: Enable the post-ra scheduler.
tstellarAMD updated this object.
tstellarAMD added a reviewer: arsenm.
tstellarAMD added a subscriber: llvm-commits.
nhaehnle added inline comments.
lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
375 ↗(On Diff #52066)

Where is this added?

lib/Target/AMDGPU/GCNHazardRecognizer.cpp
40–46 ↗(On Diff #52066)

It seems RegDefs isn't actually used anywhere?

116–117 ↗(On Diff #52066)

Are you sure this does the right thing? According to the comment, definesRegister only returns true if the instruction defines a super-register of Reg, but there is probably also a hazard if it defines a sub-register.

lib/Target/AMDGPU/SIInstrInfo.cpp
1198 ↗(On Diff #52066)

Typo: ds_read2

tstellarAMD added inline comments.Apr 4 2016, 2:39 PM
lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
375 ↗(On Diff #52066)
lib/Target/AMDGPU/GCNHazardRecognizer.cpp
116–117 ↗(On Diff #52066)

I think to correctly handle physical registers I should be using 'modifiesReigster' instead of 'definesRegister'.

For virtual registers the two functions behave the same, and I think this is OK, because when sub-registers are defined by an operand, MO.getReg() will return the super-register and not the sub-register. So MI->defineRegister(Reg) will return true if MI defines a sub-reg of Reg.

Use MI->modifiesRegister instead of definesRegister. Also move most of
the EmitInstruction() code into AdvanceCycle() which better matches
how the hazard recognizer is used by the scheduler.

tstellarAMD marked an inline comment as done.Apr 7 2016, 9:29 AM
arsenm added inline comments.Apr 21 2016, 6:44 PM
lib/Target/AMDGPU/GCNHazardRecognizer.cpp
107 ↗(On Diff #52937)

Typo recogiznizer

138–139 ↗(On Diff #52937)

A comment about why SI only would be helpful

lib/Target/AMDGPU/GCNHazardRecognizer.h
18–21 ↗(On Diff #52937)

I think only <list> is needed here. The others aren't used or can be moved into the cpp file

30 ↗(On Diff #52937)

final

32 ↗(On Diff #52937)

Typo: variabled

47 ↗(On Diff #52937)

Grammar: instruction be cycle

lib/Target/AMDGPU/SIInstrInfo.cpp
828 ↗(On Diff #52937)

line break

tstellarAMD marked 8 inline comments as done.

Address code review comments.

nhaehnle accepted this revision.Apr 27 2016, 4:26 PM
nhaehnle added a reviewer: nhaehnle.

LGTM

This revision is now accepted and ready to land.Apr 27 2016, 4:26 PM
This revision was automatically updated to reflect the committed changes.