Page MenuHomePhabricator

[AMDGPU] Fixed hazard recognizer to walk predecessors
ClosedPublic

Authored by rampitec on Jan 18 2019, 11:09 AM.

Details

Summary

Fixes two problems with GCNHazardRecognizer:

  1. It only scans up to 5 instructions emitted earlier.
  2. It does not take control flow into account. An earlier instruction

from the previous basic block is not necessarily a predecessor.
At the same time a real predecessor block is not scanned.

The patch provides a way to distinguish between scheduler and
hazard recognizer mode. It is OK to work with emitted instructions
in the scheduler because we do not really know what will be emitted
later and its order. However, when pass works as a hazard recognizer
the schedule is already finalized, and we have full access to the
instructions for the whole function, so we can properly traverse
predecessors and their instructions.

Diff Detail

Repository
rL LLVM

Event Timeline

rampitec created this revision.Jan 18 2019, 11:09 AM
arsenm added inline comments.Jan 18 2019, 12:16 PM
lib/Target/AMDGPU/GCNHazardRecognizer.cpp
288 ↗(On Diff #182563)

llvm:: not necessary

295–296 ↗(On Diff #182563)

isInlineAsm/isImplicitDef

334 ↗(On Diff #182563)

These are used a bunch of places, so could use a typedef

lib/Target/AMDGPU/GCNHazardRecognizer.h
34–35 ↗(On Diff #182563)

I'm surprised this is necessary. To clarify did you somehow only see these problems with -O0? As far as I know we don't run the standalone hazard recognizer when the scheduler is in use, so you should end up with the same issues either way.

rampitec marked an inline comment as done.Jan 18 2019, 12:20 PM
rampitec added inline comments.
lib/Target/AMDGPU/GCNHazardRecognizer.h
34–35 ↗(On Diff #182563)

The problem exists regardless of the optimization level.
We do run standalone recognizer even when scheduler is in use. We add it from GCNPassConfig::addPreEmitPass().
In fact that is wrong to call it regardless of the scheduler, as scheduler sends regions in a bottom-up order. To ensure all hazards are correctly checked a target must run standalone recognizer, there is even a comment there about it.

rampitec updated this revision to Diff 182587.Jan 18 2019, 12:38 PM
rampitec marked 3 inline comments as done.

Addressed review comments.

rampitec marked an inline comment as done.Jan 18 2019, 12:38 PM
rampitec marked an inline comment as done.Jan 19 2019, 12:17 AM
rampitec added inline comments.
lib/Target/AMDGPU/GCNHazardRecognizer.h
34–35 ↗(On Diff #182563)

It is wrong NOT to call it of course.

arsenm accepted this revision.Jan 19 2019, 2:27 PM

LGTM

This revision is now accepted and ready to land.Jan 19 2019, 2:27 PM
vpykhtin accepted this revision.Jan 21 2019, 9:02 AM

LGTM, thanks!

This revision was automatically updated to reflect the committed changes.