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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Scalar/SROA.h:92
 class SROAPass : public PassInfoMixin<SROAPass> {
+  const bool PreserveCFG;
   LLVMContext *C = nullptr;
----------------
arsenm wrote:
> Move to last field?
I'm guessing you meant "to the end of this var decl group?"


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


================
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);
----------------
arsenm wrote:
> 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)
No, nothing is missing, this is just defensive coding.

If we can't modify cfg, then we really won't get here,
and this just confirms that invariant.

E.g. we won't pass `DTU` into call to `rewriteSelectInstLoads()`
if we are not allowed to perform CFG changes.

And earlier, see `if (PreserveCFG) return {};` bailouts in `isSafeSelectToSpeculate()`.


================
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: {{.*}}
----------------
arsenm wrote:
> Just remove the prefixes?
Unless you really insist, i'd like to keep this as is.
If i remove the prefixes,
1. the tests become unconsistent. because currently they all have the same run lines and check prefixes
2. Should the run lines start producing different outputs, without this, the output will get silently lost, and requires manual changes to unbreak

The latter is really painful in e.g. codegen tests.
I would really really prefer to avoid creating that problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138238