[PATCH] D51742: [AMDGPU] Fix regression with not maintaining MachineDominatorTree

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

In https://reviews.llvm.org/D51742#1226311, @scott.linder wrote:

> The regression was caught by the expensive_checks build, if that is sufficient for testing? I am actually not sure how to write a lit test for this, because `opt -analyze` will just re-calculate the DT; I don't know how to just print the DT after the legalize pass runs.






================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:3708
+  MDT.addNewBlock(RemainderBB, LoopBB);
+  for (auto &Succ : RemainderBB->successors())
+    if (MDT.dominates(&MBB, Succ))
----------------
Braces


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:3709-3711
+    if (MDT.dominates(&MBB, Succ))
+      MDT.changeImmediateDominator(Succ, RemainderBB);
+
----------------
I think this is right, but does the new dominator tree update API support MachineDominatorTrees yet?


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:4043
+void SIInstrInfo::moveToVALU(MachineInstr &TopInst,
+                             MachineDominatorTree &MDT) const {
   SetVectorType Worklist;
----------------
This should be an optional operand since we could want to use this in the future from a pass that doesn't require the dominator tree


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:3677
+  // incorrect due to the added control-flow.
+  for (auto &MO : MI.uses())
+    if (MO.isReg() && MO.isUse())
----------------
Braces


https://reviews.llvm.org/D51742