[PATCH] D36115: [Loop Vectorize] Block Probability for Predicated Blocks

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

I'm just curious, did you run any benchmarks with this change? I think it's quite clear that using the block frequencies is a good idea, but it would be awesome to know if/by how much it improves things :)

Also, is it worth adding a dedicated test case making sure the frequencies are used by the cost model as expected?



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:343
 ///
 /// TODO: We should use actual block probability here, if available. Currently,
 ///       we always assume predicated blocks have a 50% chance of executing.
----------------
It looks like this commit fixes this TODO, remove the TODO?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:346
+
+static unsigned getReciprocalPredBlockProb1(BlockFrequencyInfo *BFI,
+                                            BasicBlock *BB,
----------------
Any reasons for the name change?

Also, given that this function takes BFI and the headerBB as arguments I think making it a member function of LoopVectorizationCostModel would make it easier to use (both BFI and HeaderBB should be available as member variables there)


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:349
+                                            BasicBlock *HeaderBB) {
+  return ((BFI->getBlockFreq(HeaderBB)).getFrequency()) /
+         ((BFI->getBlockFreq(BB)).getFrequency());
----------------
At least the outermost brackets can be omitted here I think


https://reviews.llvm.org/D36115