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

[Jessica Clarke via Phabricator via llvm-commits]

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


================
Comment at: llvm/test/CodeGen/RISCV/loop-alignment.ll:13
+
+; ALIGN_16-LABEL: test:
+; ALIGN_16:         .p2align 4
----------------
MaskRay wrote:
> jrtc27 wrote:
> > - not _ in check prefixes
> I find that `_` in check prefixes is also popular.
> 
> It has the benefit that `_` cannot conflict with `-NOT` -LABEL` etc.
I have never seen it before and there are zero uses of it in RISC-V CodeGen tests. Please conform to the existing style by using -.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106701