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

Alexey Bataev via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed May 3 11:46:33 PDT 2023

ABataev accepted this revision.
ABataev added inline comments.
This revision is now accepted and ready to land.

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:
> > > 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.

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list