[PATCH] D40973: [LV] Remove unnecessary DoExtraAnalysis guard (silent bug)

[Diego Caballero via Phabricator via llvm-commits]

dcaballe marked an inline comment as done.
dcaballe added a comment.

Thanks for the quick response, Florian!

I had a test but it didn't convince me. In order to test that a loop without PH is hitting this check we would have to match the ORE message:

  ORE->emit(createMissedAnalysis("CFGNotUnderstood")
            << "loop control flow is not understood by vectorizer");

In order to emit this message, we have to use -pass-remarks-analysis=loop-vectorize, which is setting DoExtraAnalysis to 'true'. This means that the test will pass with and without this fix :)
In addition, the message "loop control flow is not understood by vectorizer" is reused 4 more times in other legal checks so the test will pass if that message is printed by a check not related to the loop PH.

However, I can add the test I have if you still think it has some value. IMO, I don't think so.
If somebody knows a more reliable way to test this, please, let me know. Of course, the right thing to do would be to replace "loop control flow is not understood by vectorizer" messages with more specific messages depending on the legal check but that is out of the scope of this fix and maybe this message was reused for a good reason.

I will wait for your response to commit the last version of the patch with or without the test.

Thanks,
Diego



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5008
               << "loop control flow is not understood by vectorizer");
-  if (DoExtraAnalysis)
       Result = false;
----------------
fhahn wrote:
> Could you check if this has the correct indentation? It looks off in the Phabricator web view.
No, the indentation was wrong. I fixed it as part of this patch because it's related to the issue. However, I could commit it separately if there is any problem with that.


https://reviews.llvm.org/D40973