[PATCH] D86977: [IRSim][IROutliner] Limit to extracting regions that only require inputs

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:204-206
+      Optional<unsigned> GVNOpt = C.getGVN(V);
+      assert(GVNOpt.hasValue() && "Expected a GVN for operand?");
+      unsigned GVN = GVNOpt.getValue();
----------------
It's ok to lean on Optional's internal assertions.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:220
+      // it is considered to not be the same value.
+      Optional<bool> ConstantMatches = constantMatches(V, GVN, GVNToConstant);
+      if (ConstantMatches.hasValue()) {
----------------
`Optional<bool>`'s can be pretty confusing. Probably better to make `constantMatches` take a `Constant*` and return a regular `bool`, and then have this be:

```
if (Constant *C = dyn_cast<Constant>(V)) {
  if (!constantMatches(C, GVN, GVNToConstant))
    return false;
  continue;
}
```


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:491
+    // each section in the set.
+    NotSame.clear();
+    CurrentGroup.findSameConstants(NotSame);
----------------
Is there a performance issue with declaring `NotSame` here? Sinking declarations helps show the reader that there are no loop-carried dependencies.


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

https://reviews.llvm.org/D86977