[PATCH] D46827: [VPlan] Add VPInstruction to VPRecipe transformation.

[Diego Caballero via Phabricator via llvm-commits]

dcaballe added a comment.

Thanks for this new version, Florian! Pretty excited to see that some inner loop tests are passing!
Satish is working on the support for outer loops in CG and we’ve been discussing your changes. We have to make sure that this patch aligns well with his next patch and also with the constraints that we stablished for this patch series and subsequent ones. Some comments in this regard:

1. We think that `VPInstructionsToVPRecipes` is still too smart for the vplan native path and bringing too much code (from CM and Legal, for example) an recipes that we would prefer to keep away from the native path until they are properly ported. Otherwise, we’d end up having the same development limitations as we have in the inner loop path. What we had in main for the Recipe to VPInstruction transition in the native path is something as simple as the code in https://reviews.llvm.org/D44338?vs=on&id=140081&whitespace=ignore-most, where `createRecipesForVPBB` is naively creating only `VPWidenMemoryInstructionRecipe`, `VPWidenRecipe` and `VPWidenPHIRecipe`. Satish will give you more details in this regard so that this patch aligns with his.

2. An example of #1 is `createBlockInMask` and `createEdgeMask`, which are currently invoked from tryToWidenMemory. In the VPlan native path we are only supporting uniform control flow so no masking is necessary at this point. For divergence control flow we plan to introduce a VPlan based predication algorithm in patch series #2 which will conflict with code.

3. If you agree with the suggestion in #1, I think it would be better to introduce the inner loop support in the VPlan native path in a separate patch. In that way, we would introduce: a) VPInstructionToVPRecipe, b) CG support (Satish) and, finally, c) Basic inner loop support, i.e., same constraints as for outer loops, same naïve approach as in a) and b) and making sure that support for both inner and outer loops are well aligned.

Of course, we also need to work to bring whatever is necessary for SLP-aware loop vectorization. We can have this discussion offline to better understand what would be missing in the VPlan native path.

Please, let me know what you think!
Satish will follow up with more information regarding the CG patch.

Thanks,
Diego



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6300
 
-  LLVM_DEBUG(
-      dbgs() << "LV: Not vectorizing. Inner loops aren't supported in the "
----------------
Maybe it would be useful to keep this debug message for the remaining inner loops that don't hit the previous condition?


https://reviews.llvm.org/D46827