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

[Tim Corringham via Phabricator via llvm-commits]

timcorringham marked 5 inline comments as done.
timcorringham added inline comments.


================
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); }
 };
----------------
foad wrote:
> Looks like this is just reformatting. Please commit separately. Maybe even clang-format the whole file first?
clang-format did this - I will commit a clang-format version first and then update this review to eliminate formatting changes.


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


================
Comment at: llvm/lib/Target/AMDGPU/SIModeRegister.cpp:136
 
+  bool SetregInserted = false;
+
----------------
foad wrote:
> Why is this needed? Was there a problem with the existing code using NumSetRegInserted?
NumSetReginserted is a STATISTIC, and as such only works when LLVM is built with asserts or debug mode enabled - which is something I hadn't realised when I wrote the original code


================
Comment at: llvm/lib/Target/AMDGPU/SIModeRegister.cpp:355
+    if ((ThisBlock == PredBlock) && (std::next(P) == E)) {
+      BlockInfo[ThisBlock]->Pred = DefaultStatus;
+      ExitSet = true;
----------------
foad wrote:
> 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.
This handles a case that should never happen but which would cause a loop if it did.


================
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)
----------------
foad wrote:
> Just reformatting?
clang-format again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82215