[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:110
+  /// -2 and -1 are assigned by the DenseMap.
+  unsigned PHINodeGVNTracker = -3;
+
----------------
paquette wrote:
> Can we assert somewhere that -2 and -1 are actually used by DenseMap?
I'm not sure where to do that? They are representing the tombstone value, and empty key value, so you can't use them as keys for other items.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1406
+      if (AdjustArgNo && Region.AggArgToConstant.count(ArgNum) == 0) {
+        ArgNum = Region.AggArgToExtracted.find(ArgNum)->second;
+        IVal = CI->getArgOperand(ArgNum);
----------------
paquette wrote:
> can you do this like
> 
> ```
> if (!AdjustArgNo) {
> // The call instruction was already removed when we created a call to an outlined function. So, we have all the arguments already. Just grab the argument.
> IVal = CI->getArgOperand(ArgNum);
> continue;
> }
> 
> if (Region.AggArgToConstant.count(ArgNum)) {
>   // The argument is/isn't a constant...
>   // do stuff
>   continue;
> }
> 
> // Other case
> 
> ```
There is a shared action between all three of the cases.  I can clean up what the checks are, but I don't think it makes sense to use this pattern to continue and then duplicate the canonical number finding below in each case.  These cases are to make sure we are getting the canonical number for the correct value, and then we can use the same code below to find the canonical number.


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

https://reviews.llvm.org/D106995