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

Vasileios Porpodas via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed May 3 11:52:05 PDT 2023

vporpo 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;
ABataev wrote:
> vporpo wrote:
> > ABataev wrote:
> > > 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.
> > > The break below already means it, better to describe why Stack is expected to be empty here.
> > I don't understand why you don't like the message "Expected empty stack, loop should exit". 
> > The precondition is that the loop should exit at this point. The fact that there is a `break` is exactly what is being checked by the assertion.
> It is obvious already, that the loop should exit.
How is it obvious? This is exactly what the assertion is checking: if the assertion triggers it means that the loop should not exit and that `break` is wrong.

Anyway, I think we have already spent too much time on this and it is not that important. I will change it to "Expected stack is epxected" as you originally proposed, even though I think such messages are not very useful.

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list