[PATCH] D124552: CodeGenPrepare: Replace constant PHI arguments with switch condition value

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:7046
+  for (const SwitchInst::CaseHandle &Case : SI->cases()) {
+    bool CheckedCase = false;
+    ConstantInt *CaseValue = Case.getCaseValue();
----------------
Is CheckedCase only used to save time (because findCaseDest() is potentially expensive)? I'd put an explanatory comment on this declaration to make its purpose more obvious.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:7054-7055
+        for (unsigned I = 0, E = PHI.getNumIncomingValues(); I != E; I++) {
+          if (PHI.getIncomingValue(I) == CaseValue &&
+              PHI.getIncomingBlock(I) == SwitchBB) {
+            // We cannot optimize if there are multiple case labels jumping to
----------------
I'd invert this to "if (!= || !=) continue;" just to reduce the indentation. IIUC, we will do that anyway in the next patch.


================
Comment at: llvm/test/Transforms/CodeGenPrepare/X86/switch-phi-const.ll:67
+  %x2 = phi i32 [ 50, %bb0 ], [ 50, %bb0 ], [ %x1, %case_42 ]
+  %x2.2 = phi i32 [ 51, %bb0 ], [ 51, %bb0 ], [ %x1, %case_42 ]
+  store i32 %x2, i32* @effect, align 4
----------------
We should have another smaller test that does have multiple phis in a block but without multiple cases. That would still be replaced, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124552