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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added inline comments.


================
Comment at: llvm/include/llvm/IR/Value.h:575-576
+  /// If \p IsInBounds is not a nullptr and at least one GEP is stripped,
+  /// IsInBounds will reflect whether the accumulated offset is inbounds or not,
+  /// otherwise the value won't be modified.
+  ///
----------------
This could better out what happens when more than one GEP is stripped, and they had different inbound-ness.


================
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,
----------------
DaniilSuchkov wrote:
> 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.
Okay.


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

https://reviews.llvm.org/D68128