[PATCH] D108656: [IROutliner] Using Canonical Values to find Corresponding Items between Regions

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/IROutliner.h:166
+  /// exists, otherwise None.
+  Optional<Value *> findCorrespondingValueIn(const OutlinableRegion &Other,
+                                             Value *V);
----------------
I'd drop the `Optional<>` and use nullptr to represent "didn't find one". That removes ambiguity around `Optional<Value*>(nullptr)`, and would improve the call sites.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1171
+
       Instruction *NewI = I->clone();
       NewI->setDebugLoc(DebugLoc());
----------------



================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1178
+
+      StoreInst *NewSI = dyn_cast<StoreInst>(NewI);
+      if (FirstFunction)
----------------
... to fix the dereference after dyn_cast without checking it for null.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1184
+      assert(CorrVal.hasValue() && "Value is none?");
+      NewSI->setOperand(0, *CorrVal);
     }
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108656