[PATCH] D106701: [clang] Add -falign-loops=N where N is a power of 2

[Alex Bradbury via Phabricator via llvm-commits]

asb added inline comments.


================
Comment at: llvm/test/CodeGen/RISCV/loop-alignment.ll:3-4
+; RUN: llc < %s -mtriple=riscv64 | FileCheck %s
+; RUN: llc < %s -mtriple=riscv64 -align-loops=16 | FileCheck %s -check-prefix=ALIGN_16
+; RUN: llc < %s -mtriple=riscv64 -align-loops=32 | FileCheck %s -check-prefix=ALIGN_32
+
----------------
MaskRay wrote:
> luismarques wrote:
> > MaskRay wrote:
> > > jrtc27 wrote:
> > > > MaskRay wrote:
> > > > > jrtc27 wrote:
> > > > > > MaskRay wrote:
> > > > > > > luismarques wrote:
> > > > > > > > Nit: it's a convention of the RISC-V backend codegen tests to wrap the RUN lines.
> > > > > > > only 86 columns. compiler-rt is even transiting to 100 column.
> > > > > > compiler-rt is not the RISC-V backend :)
> > > > > Wrapping lines here just makes the code less readable.
> > > > That's your personal opinion, which I disagree with, and it's not true if your terminal isn't wide enough. Going against existing convention in the backend tests should only be done with very good reason, and personal opinion is not that.
> > > Lines longer than 80-column (in this case just 86) are pretty common among tests. I really hope test/CodeGen/RISCV/ can be more tolerant on this matter.
> > > 
> > > Even the Linux scripts/checkpatch.pl has increased the limit to 100 because in many cases wrapping lines for strict 80-conformance just harms readability.
> > > 
> > > Of course I don't want to waste time arguing on this matter. So if this turns out to be an issue for RISC-V folks, I'll update it to save my time.
> > > Of course I don't want to waste time arguing on this matter. So if this turns out to be an issue for RISC-V folks, I'll update it to save my time.
> > 
> > Personally, I don't particularly care. I don't know if @asb has strong feelings about this. If you think it would be beneficial to relax this convention please raise the issue on llvm-dev. Let's not keep discussing this in every patch touching RISC-V :-)
> Personally I don't even think the generic case needs to be raised on llvm-dev:) There are just so many column>80 cases in llvm/test and clang/test. Actually, If someone wants to enforce the 80-column rule more rigidly, that probably needs a discussion.
> 
> That said, the argument here is about a subdirectory: llvm/test/CodeGen/RISCV/ ...
> 
> 
I don't have a strong view on this one to be honest. I think I've typically wrapped at 80 columns for these RUN lines after being asked to, but ultimately I think choosing a logical point to split has a greater impact on readability than keeping it strictly to 80 columns.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106701