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.