[PATCH] D106995: [IROutliner] Allowing PHINodes in Exit Blocks

[Andrew Litteken via Phabricator via llvm-commits]

AndrewLitteken added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:105
 
+  unsigned PHINodeGVNTracker = -3;
+
----------------
paquette wrote:
> Why this number?
It has to count backwards from -3 (or rather the unsigned version) since -2, and -1 are reserved as tombstone and empty values for unsigned integers in the DenseMap.  I'll add it as a comment.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1060
+    GVN = *C.getCanonicalNum(GVN);
+    PHIGVNs.push_back(GVN);
+  }
----------------
paquette wrote:
> actually, can this loop be merged into the previous one?
yes, I think so.  But I think enough values would need to be passed into the lambda for this that I don't think it makes sense to put it in `any_of` if that's the case.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1066
+  Optional<unsigned> BBGVN = C.getGVN(PHIBB);
+  BBGVN = C.getCanonicalNum(BBGVN.getValue());
+  // Create a pair of the exit block canonical value, and the aggregate
----------------
paquette wrote:
> what happens if `BBGVN` doesn't have a value?
It shouldn't since the block of a Phi node containing should be the target of some branch in the region, and will have been marked in that case, but I will add an assertion to make sure it exists.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:2254
+      Optional<unsigned> OGVN = Candidate.fromCanonicalNum(OutputCanon);
+      Optional<Value *> OV = Candidate.fromGVN(*OGVN);
+
----------------
paquette wrote:
> What happens if `OVGN` doesn't have a value?
Right now, it will throw an assertion in the Optional code that there isn't a value.


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

https://reviews.llvm.org/D106995