[PATCH] D101430: [AMDGPU] Refactor hazard recognition IsHazardFn and IsExpiredFn

[Carl Ritson via Phabricator via llvm-commits]

critson added a comment.

In D101430#2721920 <https://reviews.llvm.org/D101430#2721920>, @foad wrote:

>> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101430/new/

https://reviews.llvm.org/D101430