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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/GCNHazardRecognizer.cpp:288
+    function_ref<bool(MachineInstr *, int WaitStates)> IsExpired,
+    llvm::DenseSet<const MachineBasicBlock *> &Visited) {
+
----------------
llvm:: not necessary


================
Comment at: lib/Target/AMDGPU/GCNHazardRecognizer.cpp:295-296
+    unsigned Opcode = I->getOpcode();
+    if (Opcode == AMDGPU::INLINEASM ||
+        I->getOpcode() == AMDGPU::IMPLICIT_DEF ||
+        I->isDebugInstr())
----------------
isInlineAsm/isImplicitDef


================
Comment at: lib/Target/AMDGPU/GCNHazardRecognizer.cpp:334
+    MachineInstr *MI,
+    function_ref<bool(MachineInstr *, int WaitStates)> IsExpired) {
+  llvm::DenseSet<const MachineBasicBlock *> Visited;
----------------
These are used a bunch of places, so could use a typedef


================
Comment at: lib/Target/AMDGPU/GCNHazardRecognizer.h:34-35
 class GCNHazardRecognizer final : public ScheduleHazardRecognizer {
+  // Distinguish if we are called from scheduler or hazard recognizer
+  bool IsHazardRecognizerMode;
+
----------------
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.


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

https://reviews.llvm.org/D56923