[PATCH] D109362: [SystemZ][z/OS] Add GOFF Support to the DataLayout
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 20 13:19:21 PDT 2021
MaskRay added inline comments.
================
Comment at: clang/test/CodeGen/target-data.c:256
+// RUN: %clang_cc1 -triple s390x-none-zos -o - -emit-llvm %s | \
+// RUN: FileCheck %s -check-prefix=ZOS
----------------
MaskRay wrote:
> anirudhp wrote:
> > MaskRay wrote:
> > > If you add so many RUN lines at once, please use unittests instead. This would cost some test execution time
> > We're essentially matching what was available for the systemz elf target (above lines) for the z/OS target. Is there a real concern for the execution time here for moving it to a separate unit test. If we did, it would seem to "stand out" for just the z/OS target.
> This is a real concern. Perhaps you can also move the SYstemZ ELF tests to a unit test.
It also doesn't appear useful to enumerate every combination when the patch doesn't touch these combination individually.
I.e. if you add z/OS condition to `arch8` code, then having a test is fine. If you don't touch it, I don't think you should enumerate `{elf,goff} * {z10,arch8,z196,arch9,...}`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109362/new/
https://reviews.llvm.org/D109362
More information about the llvm-commits
mailing list