[PATCH] D101875: [RISCV] Prefer to lower MC_GlobalAddress operands to .Lfoo$local

Tue May 11 10:45:16 PDT 2021

jrtc27 added inline comments.

================
Comment at: llvm/test/CodeGen/RISCV/elf-preemption.ll:1-3
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv64 -relocation-model=static < %s | FileCheck %s --check-prefix=STATIC
+; RUN: llc -mtriple=riscv64 -relocation-model=pic < %s | FileCheck %s --check-prefix=PIC
----------------
jrtc27 wrote:
> MaskRay wrote:
> > jrtc27 wrote:
> > > jrtc27 wrote:
> > > > jrtc27 wrote:
> > > > > MaskRay wrote:
> > > > > > jrtc27 wrote:
> > > > > > > MaskRay wrote:
> > > > > > > > jrtc27 wrote:
> > > > > > > > > luismarques wrote:
> > > > > > > > > > Nit: we generally add both riscv32 and riscv64 (if applicable, generally naming the prefix appropriately). I'm not quite sure where to draw the line to ensure that doesn't become silly (@jrtc27 opinions?), but in this case it looks like the codegen might be the same for both subarchs, so it might not even require any additional clutter.
> > > > > > > > > > Also, split the long RUN lines, like in other tests.
> > > > > > > > > GOT loads will differ in width, and stack spilling will differ in the few tests that end up doing that. RV32 tests should be redundant, but they should be so quick it does no harm to add them.
> > > > > > > > From the perspective of orthogonal tests, adding riscv32 provides very little value. The GOT load test has been done by other tests (e.g. pic-models.ll).
> > > > > > > Sure, but what's the downside of adding it? It's just two quick RUN lines. I take the view you should always test both unless there's a good reason not to, and laziness is not one of them.
> > > > > > Disagree with laziness.
> > > > > > 
> > > > > > The downside is the test time & readability. Duplicating RUN lines for nearly every test will unnecessarily increase test time.
> > > > > > CodeGen/RISCV, 886 files, 1029044 lines, lines/files is much larger than any other target.
> > > > > > 
> > > > > > There are dedicated tests for GOT. We should not add un-orthogonal tests to too many files.
> > > > > 90% of which are RISC-V vector tests. These kinds of tiny tests are *not* the problem and take fractions of a second to run, it's the huge pile of vector tests.
> > > > But whether or not you're willing to add RV32 tests, you still need to split your RUN lines so they're formatted like all the others.
> > > (917170 out of 1022002 lines, ie 89.7%, to be precise, in my checkout)
> > > But whether or not you're willing to add RV32 tests, you still need to split your RUN lines so they're formatted like all the others.
> > 
> > I don't understand. What's the style you prefer?
> Like pic-models.ll:
> 
> ```
> ; RUN: llc -mtriple=riscv32 -relocation-model=static < %s \
> ; RUN:     | FileCheck -check-prefix=RV32-STATIC %s
> ; RUN: llc -mtriple=riscv32 -relocation-model=pic < %s \
> ; RUN:     | FileCheck -check-prefix=RV32-PIC %s
> ; RUN: llc -mtriple=riscv64 -relocation-model=static < %s \
> ; RUN:     | FileCheck -check-prefix=RV64-STATIC %s
> ; RUN: llc -mtriple=riscv64 -relocation-model=pic < %s \
> ; RUN:     | FileCheck -check-prefix=RV64-PIC %s
> ```
But wrapped like that

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101875