This is an archive of the discontinued LLVM Phabricator instance.

[LV] Improving "Control-Flow-Select" Vectorization remark.
Needs ReviewPublic

Authored by cs15btech11044 on Mar 3 2018, 12:03 AM.

Details

Summary

Improves the "Control-Flow cannot be substituted for a select" optimization remark.

The patch provides precise instruction debug location at which this remark occurs and the remark message has also been changed for better clarity, namely "if-conversion is not possible due to lack of support of predication". A separate test case has also been added for this purpose, on top of some modifications in the existing test cases.

Diff Detail

Event Timeline

cs15btech11044 created this revision.Mar 3 2018, 12:03 AM
cs15btech11044 edited the summary of this revision. (Show Details)
cs15btech11044 set the repository for this revision to rL LLVM.
cs15btech11044 edited the summary of this revision. (Show Details)
cs15btech11044 retitled this revision from Improving "Control-Flow-Select" Vectorization remark. to [LV] Improving "Control-Flow-Select" Vectorization remark..
cs15btech11044 added a reviewer: vivekvpandya.

Few high level comments from my side. For more accurate review I recommend wait until @anemet takes loot at it :)

  1. Is there any bug reported for this? if yes then please mention that.
  2. I don't understand the purpose of new test case when existing test case already captures this remarks. if any please explain reason to add new test case in description.
  3. I hope in addition to LIT based testing you have also done unit testing. You need to enable unit testing with cmake while building LLVM source tree with cmake flag LLVM_BUILD_TESTS=On and then with check-all target in LLVM build system, you can use make check-all or ninja check-all for running those tests.
lib/Transforms/Vectorize/LoopVectorize.cpp
1763

Instread of Global Valriable I think it is better to use SelectRemarkInstruction as function parameter to blockCanBePredicated() which will be set when it returns false.

5702–5703

Please add space between ) and { so that is consistent with rest of the code.
This applies for all new if statements you added.

5711

Does using LI as SelectRemarkInstruction provide any more information to optimization remark? if not then I suggest using I. Same thing applies to SI at line no 5732, 5748.

cs15btech11044 marked 3 inline comments as done.

Is there any bug reported for this? if yes then please mention that.

Well if there is no bug reported for this then do you have any motivating example or other community member has suggested this improvement?

lib/Transforms/Vectorize/LoopVectorize.cpp
4838

@anemet is this message ok? I would prefer "if-conversion was not possible" or previous version of it. also should we also change remark title NoCFGForSelect to IfConversionNotPossible ?

5721

Are you fetching your LLVM repository with SVN? It seems that https://github.com/llvm-mirror/llvm/blob/54cff77be9a4f9758a82381f9f12f82f59e3bb48/lib/Transforms/Vectorize/LoopVectorize.cpp#L5690 is not updated code.
Because there is difference in code between your revision and llvm-mirror 's LV.cpp
But please make sure that all your changes are with respect to TOT (top of the tree) of LLVM's SVN repository.

test/Transforms/LoopVectorize/X86/vectorization-remarks-missed.ll
48

Earlier revision has line number 27 as debug location of this remark. I think that is the main purpose of this patch. Could you please verify if this change is correct or not?

hsaito added a subscriber: hsaito.Mar 19 2018, 10:04 AM
hsaito added inline comments.
lib/Transforms/Vectorize/LoopVectorize.cpp
5704–5705

Here, we can generate a very crisp message, right? "Conditionally used constant can cause a trap. Try manually hoisting it above the condition(s)."

We could generate vector code like below (naive code for illustrative purpose, may not be good for branch prediction) and still be safe, but that could be for another patch.

if (mask is not all false) {

splatvector = splat(C)

}
else {

splatvector = splay(undef);

}

5753–5754

Here, we should generate a crisp message "Instruction may throw an exception. Try disabling exception handling."

There are a few issues with formatting (trailing spaces, using tabs, lines > 80 chars, see https://llvm.org/docs/CodingStandards.html). Could you use clang-format to format the diff?

lib/Transforms/Vectorize/LoopVectorize.cpp
1726

You could use a reference to an instruction pointer here I think (Instruction *&SelectRemarkInstruction). This would simplify the code slightly (a few less * and &) with the bonus of not being able to pass in nullptr).

4835–4838

I think we should initialize this with nullptr. So things fall over quickly if we miss a case in blockCanBePredicated.

There are a few issues with formatting (trailing spaces, using tabs, lines > 80 chars, see https://llvm.org/docs/CodingStandards.html). Could you use clang-format to format the diff?

I may have missed that. But just to inform @cs15btech11044 that run clang-format on your new changes only, keep existing code as it is even if clang-format may format it.

cs15btech11044 marked an inline comment as done.Mar 21 2018, 5:25 AM

Hi all,
Sorry for this delayed response

Is there any bug reported for this? if yes then please mention that.

Well if there is no bug reported for this then do you have any motivating example or other community member has suggested this improvement?

There is no bug reported for this remark. The reason for improving the remark message was that the original remark message "Control Flow cannot be substituted for a select" was a bit obscure, when viewed by someone who has basic familiarity with LLVM. Instead, the new remark message, "if-conversion not possible due to lack of support for predication" is more relatable to the source code, thereby giving much clearer analysis into vectorization remark.

I hope this answers your question. Further remarks are also welcome.