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

[Carl Ritson via Phabricator via llvm-commits]

critson added inline comments.


================
Comment at: llvm/trunk/lib/Target/AMDGPU/GCNHazardRecognizer.cpp:317
+    if (IsExpired(nullptr, MinWaitStates))
+      return MinWaitStates;
+
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56923