[PATCH] D68128: [InstCombine] Fold PHIs with equal incoming pointers

[Daniil Suchkov via Phabricator via llvm-commits]

DaniilSuchkov added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:1144-1155
+  for (Use &Incoming : PN.incoming_values()) {
+    if (!isa<Instruction>(Incoming))
+      return nullptr;
+    int64_t CurrentOffset;
+    bool IsCurrentInBounds = true;
+    Value *CurrentBase = GetPointerBaseWithConstantOffset(
+        Incoming, CurrentOffset, DL, /* AllowNonInbounds */ true,
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > This still looks like a hoisting problem to me, honestly.
> > 
> > In particular, why do we want *all* the incoming values to be identical?
> > I.e. why can't we track *which* values are identical,
> > and if there are two-or-more of of particular value hoist+common them?
> To be more specific
> ```
> struct ZZZ {
>   Value *Base;
>   int64_t Offset;
>   bool IsInBounds;
> }
> 
> DenseMap<ZZZ, SmallVector<Value *, 4>> QQQ;
> 
> for (Use &Incoming : PN.incoming_values()) {
>   QQQ[GetPointerBaseWithConstantOffset(Incoming)].append(Incoming);
> }
> 
> for(??? I : QQQ) {
>   if(I.second.size() < 2)
>     continue;
>   <hoist>
>   replaceInstUsesWith()
> }
> ```
> 
> This still looks like a hoisting problem to me, honestly.
> 
> In particular, why do we want *all* the incoming values to be identical?
> I.e. why can't we track *which* values are identical,
> and if there are two-or-more of of particular value hoist+common them?

Because the main idea here is to get rid of a redundant PHI, since it makes the code analyzable by optimizations that can't (and in general don't need to) walk through PHIs. Yes, it also makes a number of identical pointers trivially equal, but it is more of a side-effect.
What you propose is more complex, has broader scope (thus is more likely to have unwanted side-effects) but it doesn't improve the solution of the initial problem.


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

https://reviews.llvm.org/D68128