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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay accepted this revision.
MaskRay 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
----------------
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.


================
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:
> 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.`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145999