[PATCH] D149627: [NFC][SLP] Cleanup: Simplify traversal loop in SLPVectorizerPass::vectorizeHorReduction().

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:14097
+        assert(Stack.empty() &&
+               "This checks that changing `continue` to `break` was correct.");
+        break;
----------------
vporpo wrote:
> ABataev wrote:
> > vporpo wrote:
> > > vporpo wrote:
> > > > ABataev wrote:
> > > > > vporpo wrote:
> > > > > > ABataev wrote:
> > > > > > > Replace the message with something meaningful, like 'Empty Stack is expected.'
> > > > > > I think we should also mention why we are checking that Stack is empty, so that we know how to fix this if it triggers the assertion. How about: "Empty Stack expected. Change `break` to `continue`?"
> > > > > The change continue to break is actual only for this patch, not for the codebase. So just `Empty stack expected, the original root is reached` or something like this should be enough
> > > > I don't understand what you mean. We are adding the assertion specifically for checking that changing continue to break was correct, and you are arguing that we should not mention this in the assertion message, and instead write some generic message that provides no information about why this assertion exists. 
> > > > How will a message like "Empty stack expected, the original root is reached" is supposed to help a third person fix the code if the assertion triggers?
> > > Anyway I don't feel to strongly about this I will change it to what you are proposing.
> > The assert is not about the change itself. It should not inform about changes you made but about the precondition expected here. asserts are for precondition/postcondition messages, not change history.
> Yes the precondition is that the loop will exit right after this point, which explains why we are using `break`. Shouldn't we mention this in the message? I just don't like assertion messages that repeat the obvious like mentioning "Expected empty stack" when the assertion is `assert(Stack.empty())`.
You don't need to describe git history in the message, better describe better why Stack is expected to be empty (`the original root is reached` should help with this). Also, check https://llvm.org/docs/CodingStandards.html#assert-liberally and message examples.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149627/new/

https://reviews.llvm.org/D149627