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

Fangrui Song via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue May 11 09:28:23 PDT 2021


MaskRay marked 2 inline comments as done.
MaskRay 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:
> > > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101875



More information about the llvm-commits mailing list