[PATCH] D138238: [SROA] For non-speculatable `load`s of `select`s -- split block, insert then/else blocks, form two-entry PHI node

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Scalar/SROA.h:92
 class SROAPass : public PassInfoMixin<SROAPass> {
+  const bool PreserveCFG;
   LLVMContext *C = nullptr;
----------------
Move to last field?


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:840
+Expected<SROAOptions> parseSROAOptions(StringRef Params) {
+  if (Params == "PreserveCFG")
+    return SROAOptions::PreserveCFG;
----------------
I thought pass options were-spelled-like-this?


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:1432-1433
+  // Transfer alignment and AA info if present.
+  TL->setAlignment(LI.getAlign());
+  FL->setAlignment(LI.getAlign());
 
----------------
I know it was already like this, but can't you just do CreateAlignedLoad to start?


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:1502
+    } else {
+      assert("Should not get here when not allowed to modify the CFG!");
+      rewriteLoadOfSelect(SI, *LI, Spec, *DTU);
----------------
Missing check if CFG modify is allowed? Could also move this out to where this is called and check the return value (which it looks like is already there)


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:4951-4953
+  OS << "<";
+  OS << (PreserveCFG ? "PreserveCFG" : "ModifyCFG");
+  OS << ">";
----------------
Why not just OS << (PreserveCFG ? "<PreserveCFG>" : "<ModifyCFG>";


================
Comment at: llvm/test/Transforms/SROA/2009-02-20-InstCombine-SROA.ll:274-276
+;; NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
+; CHECK-ModifyCFG: {{.*}}
+; CHECK-PreserveCFG: {{.*}}
----------------
Just remove the prefixes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138238