[PATCH] D126525: [SSAUpdaterImpl] Do not generate phi node with all the same incoming values

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/SSAUpdaterImpl.h:332
+    BBInfo *PredInfo = Info->Preds[0];
+    assert(PredInfo && "Pred Info?");
+    if (!PredInfo->AvailableVal)
----------------
skatkov wrote:
> sameerds wrote:
> > Is this assert really needed? For someone unfamiliar with this code (like me), seeing this assert creates a doubt about the surrounding code ... as if there are complicated paths reaching here, where it might be possible that the BBInfo //may// be in an invalid state at this point.
> This is more or less paranoid. The comment for BBInfo class states that predecessor info is a cache to speedup predecessor traversal.
> So with this assert I'd like to ensure that the cache is filled. If it confuses I can remove an assert.
Yeah, I think we should not have this assert. Cache or not, it is quite reasonable to bravely work under the assumption that the array contains valid elements if NumPreds is non-zero. If the cache were unreliable, then there should be a function such as "fetchPreds" or something, to be called by any point in code where the preds must be valid.


================
Comment at: llvm/include/llvm/Transforms/Utils/SSAUpdaterImpl.h:347
+    BBMap[Info->BB]->AvailableVal = Singular;
+    assert(Info->AvailableVal && "Missed assignement?");
+    return true;
----------------
skatkov wrote:
> sameerds wrote:
> > When is "Info->AvailableVal" assigned? If it's the same as the previous statement, then the implicit identity relationship between BBMap[Info->BB] and Info is quite confusing. Can this function be rewritten to consistently use just one of these two expressions?
> > When is "Info->AvailableVal" assigned? If it's the same as the previous statement, then the implicit identity relationship between BBMap[Info->BB] and Info is quite confusing. Can this function be rewritten to consistently use just one of these two expressions?
> 
> Incoming Info object comes from BlockList which is filled in BuildBlockList where BBMap is filled. So in reality BBMap[Info->BB] == Info and this is something complicated traversal, so I've added the assert above.
> 
> I wrote the code in this way to ensure that all "outer" data structures are filled.
I don't think there is any side-effect in the BBMap container other than this one pointer being updated. I think it's more readable to update only Info. If it is useful to be paranoid, then the assert should be on "BBMap[Info->BB] == Info".


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

https://reviews.llvm.org/D126525