[PATCH] D56923: [AMDGPU] Fixed hazard recognizer to walk predecessors

Wed Apr 28 01:16:57 PDT 2021

critson added inline comments.

================
Comment at: llvm/trunk/lib/Target/AMDGPU/GCNHazardRecognizer.cpp:317
+    if (IsExpired(nullptr, MinWaitStates))
+      return MinWaitStates;
+
----------------
critson wrote:
> rampitec wrote:
> > foad wrote:
> > > I think this early return 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.)
> > Hm... Probably. Did you try to exploit it?
> 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.
See D101430.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56923