This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][NFC] Refactor hazard recognition IsHazardFn and IsExpiredFn
ClosedPublic

Authored by critson on Apr 28 2021, 1:08 AM.

Details

Summary

Refactor IsHazardFn and IsExpiredFn to use constant references as these should not be mutating the instructions visited and the instruction can never be null.

Diff Detail

Event Timeline

critson created this revision.Apr 28 2021, 1:08 AM
critson requested review of this revision.Apr 28 2021, 1:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2021, 1:08 AM
foad added a comment.Apr 28 2021, 1:21 AM

Copying some of the prior discussion over from D56923:

@foad wrote:

I think this early return [the one being removed in this patch] is broken, because there might be another predecessor that has a smaller value of MinWaitStates which would not satisfy the IsExpired test. Do you agree? (Sorry for reopening such an old review.)

@critson wrote:

I do not think it is broken per-say but definitely confusing.
This allows the IsExpired function to trigger an early exit given a specific waitstate count.
The early-exit probe is detectable as it occurs with the MachineInstr* set to null.
And hence avoidable by return false if !MI.

Looking at all the IsExpired implementations, none of them use this functionality (or at least not correctly).
So I'll prepare a patch to remove it and tidy up.

foad added a comment.Apr 28 2021, 1:27 AM

I do not think it is broken per-say but definitely confusing.

I think it is broken because we are looping over predecessors trying to find the most recent hazard. The early-out allows the search to terminate if the hazard in some predecessor was too long ago, but then we would miss the fact that another predecessor might have a more recent hazard.

Looking at all the IsExpired implementations, none of them use this functionality (or at least not correctly).

But I think they do? Notably the one in getWaitStatesSince(IsHazardFn IsHazard, int Limit):

auto IsExpiredFn = [Limit] (MachineInstr *, int WaitStates) {
  return WaitStates >= Limit;
};

But also the one in checkFPAtomicToDenormModeHazard.

I do not think it is broken per-say but definitely confusing.

I think it is broken because we are looping over predecessors trying to find the most recent hazard. The early-out allows the search to terminate if the hazard in some predecessor was too long ago, but then we would miss the fact that another predecessor might have a more recent hazard.

I think what I meant was that if the caller required the closest hazard then it needs to supply IsExpired that ignores the nullptr probe, e.g.

auto IsExpiredFn = [Limit] (MachineInstr *MI, int WaitStates) {
  return MI && WaitStates >= Limit;
};

Looking at all the IsExpired implementations, none of them use this functionality (or at least not correctly).

But I think they do? Notably the one in getWaitStatesSince(IsHazardFn IsHazard, int Limit):

auto IsExpiredFn = [Limit] (MachineInstr *, int WaitStates) {
  return WaitStates >= Limit;
};

But also the one in checkFPAtomicToDenormModeHazard.

Yes, I wrote the comment before I noticed all the small ones potentially using the early exit while refactoring.

foad added a comment.Apr 28 2021, 1:38 AM

I do not think it is broken per-say but definitely confusing.

I think it is broken because we are looping over predecessors trying to find the most recent hazard. The early-out allows the search to terminate if the hazard in some predecessor was too long ago, but then we would miss the fact that another predecessor might have a more recent hazard.

I think what I meant was that if the caller required the closest hazard then it needs to supply IsExpired that ignores the nullptr probe, e.g.

auto IsExpiredFn = [Limit] (MachineInstr *MI, int WaitStates) {
  return MI && WaitStates >= Limit;
};

Surely all users always want the closest (most recent) hazard? I can't imagine the use case for anything else.

I still hope there is a way to fix the early-out case, rather than removing it completely, but I have not worked through the details yet.

I still hope there is a way to fix the early-out case, rather than removing it completely, but I have not worked through the details yet.

I don't think you can find the minimum wait count without a breath-first search?
Unless you are suggesting we allow selected searches to complete early?

This looks like 3 different changes to me: NFC formatting change, const change and IsExpired removal. Looks good in principle unless I have missed something behind first two changes.

foad added inline comments.Apr 29 2021, 2:18 AM
llvm/lib/Target/AMDGPU/GCNHazardRecognizer.cpp
430–431

Isn' t this dead code? We can only get here if one of the recursive calls to getWaitStatesSince returned a finite value, but then the recursive call will already have done the IsExpired test and returned INTMAX if it failed.

In any case, how about just deleting these three lines as a first patch, and then all the other NFC changes can be done as follow ups?

critson marked an inline comment as done.Apr 29 2021, 4:57 AM

Move the code removal change to D101520, making this NFC.

critson retitled this revision from [AMDGPU] Refactor hazard recognition IsHazardFn and IsExpiredFn to [AMDGPU][NFC] Refactor hazard recognition IsHazardFn and IsExpiredFn.Apr 29 2021, 4:58 AM
critson edited the summary of this revision. (Show Details)
foad accepted this revision.Apr 29 2021, 6:30 AM
This revision is now accepted and ready to land.Apr 29 2021, 6:30 AM
This revision was landed with ongoing or failed builds.Apr 29 2021, 5:19 PM
This revision was automatically updated to reflect the committed changes.