[PATCH] D38133: [CGP] Make optimizeMemoryInst introduce a select/phi if it improves things

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4388
+      return nullptr;
+    PHINode *NewPHI = PHINode::Create(T, OrigPHI->getNumIncomingValues(),
+                                      OrigPHI->getName(),
----------------
skatkov wrote:
> What if this Phi already exists?
I think this case where a single PHI or Select needs inserted (or reused) is a enough to require the appropriate structure in the caller.  I'd suggest that we factor out a common patch which does this which can implement both your patch and Serguei's analysis behind the same API.  Once we have that, the code structure is in place for either patch and we can focus on reviewing the two analyses.  


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4498
+    // Similar for select.
+    if (SelectInst *SI = dyn_cast<SelectInst>(V)) {
+      worklist.push_back(SI->getFalseValue());
----------------
I think the select handling can be separate from the rest of the patch and reviewed separately.  This should kick in even if the underlying addresses share the same addr mode.

Please do this to simply the review and increase the common code which can be shared between the two related patches in this area.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4558
+        DifferCount == 1) {
+      AddrModes.emplace_back(NewAddrMode);
+      continue;
----------------
Having an unbounded number of AddrModes here is likely problematic.  How much power do you loose if you cap this to say 2 or 3?


Repository:
  rL LLVM

https://reviews.llvm.org/D38133