[PATCH] D145999: [RISCV] Reserve X18 by default for Android

Tue Mar 14 23:22:43 PDT 2023

hiraditya added inline comments.

================
Comment at: clang/test/Driver/riscv-fixed-x-register.c:343
+
+// Check that x18 is reserved on Android by default
+// RUN: %clang --target=riscv64-linux-android -### %s 2> %t
----------------
MaskRay wrote:
> MaskRay wrote:
> > asb wrote:
> > > samitolvanen wrote:
> > > > This seems redundant. Isn't the LLVM codegen test sufficient here?
> > > I think testing that the `+reserve-x18` target feature is added by the frontend probably has some value.
> > > 
> > > I think it would better match the rest of this file to just do `--check-prefix=CHECK-FIXED-X18` (I _think_ that works, but it is the end of the day so I may be missing something obvious...).
> > > 
> > > Please do try to remember to generate patches with full context (`-U999999` or similar).
> > I prefer shared CHECK prefixes `--check-prefix=CHECK-FIXED-X18` as well. We do that for many other tests.
> `/// Check that x18 is reserved on Android by default.`
@samitolvanen correct, this test isn't possible because llvm implicitly reserves the register so clang -cc1 never gets the `"-target-feature" "+reserve-x18"` parameter.  I'll remove this test.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145999