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

Sat Jul 24 14:12:48 PDT 2021

MaskRay marked an inline comment as done.
MaskRay 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
+
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106701