[PATCH] D89798: [SVE][AArch64] Fix TypeSize warning in loop vectorization legality

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:1020
+        continue;
+      if (isDereferenceableAndAlignedInLoop(LI, TheLoop, SE, *DT))
         SafePointers.insert(LI->getPointerOperand());
----------------
david-arm wrote:
> peterwaller-arm wrote:
> > peterwaller-arm wrote:
> > > sdesmalen wrote:
> > > > peterwaller-arm wrote:
> > > > > I'd be tempted to make this if (!foo()) continue, so that the "passing" case is in the body of the loop, thus making it straightforward to add or remove conditions from the loop.
> > > > nit: yet now the diff is bigger, it seems simpler to just add one extra line:
> > > > ```
> > > >       if (LI && !mustSuppressSpeculation(*LI) &&
> > > > +         !LI->getType()->isVectorType() &&
> > > >           isDereferenceableAndAlignedInLoop(LI, TheLoop, SE, *DT))```
> > > On this occasion he was following https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
> > > 
> > > Is there an etiquette to minimize size-of-diff?
> > I guess that would be covered by the introduction [0] to the guide. It explicitly says that large scale refactoring isn't wanted, but that if you're changing it anyway then update it. My thinking is that that this sort of small patch it would be legitimate to update the style in the surrounding to follow the guide with respect to the early exit recommendation.
> > 
> > So what do others think on balance: legitimate in this case or not?
> > 
> > [0] https://llvm.org/docs/CodingStandards.html#introduction
> I understand where you're coming from here @peterwaller-arm and there have been occasions in the past where reviewers have also asked me to redo code to make things a bit clearer and follow a preferred style. I don't think it's wrong to undertake small scale refactoring. I guess it's just in this particular case the early-exit link you posted above gives an example where the if block is very large, i.e. the docs say "This code has several problems if the body of the 'if' is large." In this particular patch the if block is a single line statement, so perhaps it's fine to have a multi-condition if statement?
One of the main benefits of rewriting to a loop to one with early exits is when there are multiple nested conditions, e.g.

   for(...) {
     if (auto *X = dyn_cast<something>(....)) 
       if (<some condition>)
         // ...
         if (<some other condition>)
           // ... hand the case
    }

Where it is better to pick those apart to avoid the multiple levels of indentation. 

With the coding standard there is some freedom of interpretation, because it doesn't give any quantifications for what a 'large' body or condition is and leaves the trade-off between "size of patch" and "refactoring clarifies the code" up to the developer. That's intentional to avoid people rewriting lots of existing code into canonical form because they need to add one line. In this particular case, because the indentation is just one level and the body of the if-statement has only one statement, I think that balance tips in the favour of the one-line change over the refactoring.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89798