[PATCH] D30103: [SLP] Rework `findBuildAggregate()` from ercursive form to iterative, NFC.

[Alexey Bataev via llvm-commits]

1. Yes, it must be nullptr, thanks, missed it.

2. Yes, it is required, otherwise compilation is crashed

-------------
Best regards,
Alexey Bataev

17.02.2017 22:13, Michael Kuperstein via Phabricator пишет:
> mkuper accepted this revision.
> mkuper added a comment.
> This revision is now accepted and ready to land.
>
> LGTM with minor comments.
>
>
>
> ================
> Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4617
>                                  SmallVectorImpl<Value *> &BuildVectorOpds) {
> -  if (!IV->hasOneUse())
> -    return false;
> -  Value *V = IV->getAggregateOperand();
> -  if (!isa<UndefValue>(V)) {
> -    InsertValueInst *I = dyn_cast<InsertValueInst>(V);
> -    if (!I || !findBuildAggregate(I, BuildVector, BuildVectorOpds))
> +  Value *V = IV;
> +  do {
> ----------------
> Any reason this isn't nullptr? It looks like you never use the initial value.
>
>
> ================
> Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4628
> +  } while (true);
> +  std::reverse(BuildVector.begin(), BuildVector.end());
> +  std::reverse(BuildVectorOpds.begin(), BuildVectorOpds.end());
> ----------------
> Does the order really matter?
> Anyway, if you think it does, I'm fine with this to keep it NFC.
>
>
> https://reviews.llvm.org/D30103
>
>
>