[PATCH] D82215: [AMDGPU] Avoid redundant mode register writes

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

Overall I think this is probably OK but (a) I'd like to see it with some of the irrelevant stuff removed and (b) there is ample scope for making this pass look much more like a "normal" dataflow solver, where Status should be a lattice with a proper "bottom" value, the phase 2 worklist should be seeded with just the entry block so we don't have to worry about visiting unreachable blocks, and hopefully processBlockPhase2 will get much simpler again.



================
Comment at: llvm/lib/Target/AMDGPU/SIModeRegister.cpp:86
 
-  bool isCombinable(Status &S) {
-    return !(Mask & S.Mask) || isCompatible(S);
-  }
+  bool isCombinable(Status &S) { return !(Mask & S.Mask) || isCompatible(S); }
 };
----------------
Looks like this is just reformatting. Please commit separately. Maybe even clang-format the whole file first?


================
Comment at: llvm/lib/Target/AMDGPU/SIModeRegister.cpp:115
+
+  BlockData() : FirstInsertionPoint(nullptr), ExitSet(false){};
 };
----------------
Just reformatting?


================
Comment at: llvm/lib/Target/AMDGPU/SIModeRegister.cpp:136
 
+  bool SetregInserted = false;
+
----------------
Why is this needed? Was there a problem with the existing code using NumSetRegInserted?


================
Comment at: llvm/lib/Target/AMDGPU/SIModeRegister.cpp:355
+    if ((ThisBlock == PredBlock) && (std::next(P) == E)) {
+      BlockInfo[ThisBlock]->Pred = DefaultStatus;
+      ExitSet = true;
----------------
If a block is its only predecessor then either it's the entry block (this wouldn't be allowed in IR but I guess it is in MIR?), or it's an unreachable single block loop, and it doesn't matter what we do in unreachable code. So the only useful thing this line and line 338 achieve is to set DefaultStatus at the start of the entry block. It seems like this could be simplified.


================
Comment at: llvm/lib/Target/AMDGPU/SIModeRegister.cpp:403-404
   if (!BlockInfo[ThisBlock]->Pred.isCompatible(BlockInfo[ThisBlock]->Require)) {
-    Status Delta = BlockInfo[ThisBlock]->Pred.delta(BlockInfo[ThisBlock]->Require);
+    Status Delta =
+        BlockInfo[ThisBlock]->Pred.delta(BlockInfo[ThisBlock]->Require);
     if (BlockInfo[ThisBlock]->FirstInsertionPoint)
----------------
Just reformatting?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82215