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

[Serguei Katkov via Phabricator via llvm-commits]

skatkov 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)
----------------
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.


================
Comment at: llvm/include/llvm/Transforms/Utils/SSAUpdaterImpl.h:338
+      BBInfo *PredInfo = Info->Preds[Idx];
+      assert(PredInfo && "Pred Info?");
+      if (!PredInfo->AvailableVal || Singular != PredInfo->AvailableVal)
----------------
sameerds wrote:
> Is this assert really needed?
> Is this assert really needed?

ditto


================
Comment at: llvm/include/llvm/Transforms/Utils/SSAUpdaterImpl.h:347
+    BBMap[Info->BB]->AvailableVal = Singular;
+    assert(Info->AvailableVal && "Missed assignement?");
+    return true;
----------------
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.


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

https://reviews.llvm.org/D126525