[CodeGen] Add AArch64 behavior to existing MFS tests

[dhoekwater]

Summary

X86 and AArch64 have some small differences in output because of non-MFS
factors (such as encoding function calls as callq vs bl). Add
AArch64 checks for tests where output varies between platforms even
if MFS logic doesn't. Add a debug flag to run MFS on any target.
Fix the encoding of nops in bbsections.

Without this change, there is no way to test that MFS functions correctly on AArch64.

[dhoekwater]

Rebase onto other MFS tests and fix the BBsections code that makes the test crash

[dhoekwater]

Don't warn for AArch64 if mfs-ignore-triple is enabled

[rahmanl]

llvm/lib/CodeGen/BasicBlockSections.cpp

294 ↗

(On Diff #551202)

Is this because of ARM? Maybe add a comment to explain the rationale.

llvm/lib/CodeGen/MachineFunctionSplitter.cpp

73 ↗

(On Diff #551202)

How about introducing a permanent option named SkipUnsupportedTriple and default it True so we can use it for any future architectures?

[dhoekwater]

llvm/lib/CodeGen/MachineFunctionSplitter.cpp

73 ↗

(On Diff #551202)

I'm onboard with making this a permanent flag for testing purposes only, but I'm not sure about inverting and defaulting to true. Isn't it standard to have default-false hidden flags that enable some special behavior (as opposed to default-true hidden flags that enable the default behavior)?

For example, most of the cl::opt<bool> options in llvm/lib/Analysis/InlineCost.cpp follow this pattern.

[rahmanl]

llvm/lib/CodeGen/MachineFunctionSplitter.cpp

73 ↗

(On Diff #551202)

Up to you. I thought this naming better matches with the current code (UnsupportedTriple and isSupportedTriple). Ignore doesn't fully convey what the flag actually does.

[dhoekwater]

Add comments

[dhoekwater]

Rename the flag to split functions on unsupported targets

[rahmanl]

llvm/lib/CodeGen/BasicBlockSections.cpp

294 ↗

(On Diff #551202)

I see, this is target specific now. Although this code was target-specific before too, but we only wanted code for X86. Do you think we should implement insertNoop (https://llvm.org/doxygen/classllvm_1_1TargetInstrInfo.html#a4ee57d5d6295dfeb44f3b55301b20020) for X86 and AArch64 now?

llvm/lib/CodeGen/MachineFunctionSplitter.cpp

71 ↗

(On Diff #551281)

Even better naming than mine!

[dhoekwater]

llvm/lib/CodeGen/BasicBlockSections.cpp

294 ↗

(On Diff #551202)

Yes! That's exactly what I was looking for, but I couldn't find the function since I was looking for something like "addNop". Should we do that in this patch or should I make another?

[rahmanl]

llvm/lib/CodeGen/BasicBlockSections.cpp

294 ↗

(On Diff #551202)

Right. Separate the entire avoidZeroOffsetLandingPad changes into its own patch if you can. You might get new reviewers when implement TargetInstrInfo.

[dhoekwater]

llvm/lib/CodeGen/BasicBlockSections.cpp

294 ↗

(On Diff #551202)

Unfortunately, we need AArch64 tests to test the avoidZeroOffsetLandingPad changes, and we need the changes so that the tests don't crash on AArch64.

[dhoekwater]

Implement the AArch64 InsertNoop hook and use it so that Arm tests don't crash

[shenhan]

llvm/lib/CodeGen/MachineFunctionSplitter.cpp

72 ↗

(On Diff #551617)

nit: "mfs" already contains "split", so mfs-split-xxx seems a little bit redundant. "mfs-allow-unsupported-triple" seems more aligned with the variable name.

150 ↗

(On Diff #551617)

Thanks for fixing the format.

[thakis]

This breaks tests on windows: http://45.33.8.238/win/82918/step_7.txt

Please take a look and revert for now if it takes a while to fix.

[thakis]

reverted in 3d22dac6c3b97d7bb92f243886dfb0d32a5c42e9 for now

[dhoekwater]

Relanded in 90ab85a1b2e72f63039fadf6669b23f52192defd

clang/test/CodeGen/fsplit-machine-functions.c

+// REQUIRES: x86-registered-target
+// REQUIRES: arm-registered-target
+// REQUIRES: nvptx-registered-target
+
+// Check -fsplit-machine-functions passed to cuda device causes a warning.
+// RUN: %clang_cc1 -triple nvptx-unknown-unknown -target-cpu sm_70 \
+// RUN:     -fcuda-is-device -x cuda -fsplit-machine-functions -S %s \
+// RUN:     -o %t 2>&1 | FileCheck %s --check-prefix=MFS1
+// MFS1: warning: -fsplit-machine-functions is not valid for nvptx
+
+// Check -fsplit-machine-functions passed to X86 does not cause any warning.
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsplit-machine-functions \
+// RUN:     -o %t -S %s && { echo "empty output causes FileCheck to fail" ; } \
+// RUN:     2>&1 | FileCheck %s --check-prefix=MFS2
+// MFS2-NOT: warning:
+
+// Check -fsplit-machine-functions passed to ARM does cause a warning.
+// RUN: %clang_cc1 -triple arm-unknown-linux-gnueabi \
+// RUN:     -fsplit-machine-functions -S %s -o %t \
+// RUN:     2>&1 | FileCheck -check-prefix=MFS3 %s
+// MFS3: warning: -fsplit-machine-functions is not valid for arm
+
+int foo() {
+  return 13;
+}

clang/test/Driver/fsplit-machine-functions-with-cuda-nvptx.c

-// REQUIRES: system-linux
 // REQUIRES: x86-registered-target
 // REQUIRES: nvptx-registered-target
-// REQUIRES: shell
 
 // Check that -fsplit-machine-functions is passed to both x86 and cuda
 // compilation and does not cause driver error.
-// RUN:   cd "$(dirname "%t")" ; \
-// RUN:   %clang -### --target=x86_64-unknown-linux-gnu -nogpulib -nogpuinc \
+// RUN: %clang -### --target=x86_64-unknown-linux-gnu -nogpulib -nogpuinc \
 // RUN:     --cuda-gpu-arch=sm_70 -x cuda -fsplit-machine-functions -S %s \
 // RUN:     2>&1 | FileCheck %s --check-prefix=MFS1
 // MFS1: "-target-cpu" "x86-64"{{.*}}"-fsplit-machine-functions"
 // MFS1: "-target-cpu" "sm_70"{{.*}}"-fsplit-machine-functions"
 
-// Check that -fsplit-machine-functions is passed to cuda and it
-// causes a warning.
-// RUN:   cd "$(dirname "%t")" ; \
-// RUN:   %clang --target=x86_64-unknown-linux-gnu -nogpulib -nogpuinc \
-// RUN:     --cuda-gpu-arch=sm_70 -x cuda -fsplit-machine-functions -S %s \
-// RUN:     2>&1 | FileCheck %s --check-prefix=MFS2
-// MFS2: warning: -fsplit-machine-functions is not valid for nvptx
-
 // Check that -Xarch_host -fsplit-machine-functions is passed only to
 // native compilation.
-// RUN:   cd "$(dirname "%t")" ; \
-// RUN:   %clang -### --target=x86_64-unknown-linux-gnu -nogpulib -nogpuinc \
+// RUN: %clang -### --target=x86_64-unknown-linux-gnu -nogpulib -nogpuinc \
 // RUN:     --cuda-gpu-arch=sm_70 -x cuda -Xarch_host \
 // RUN:     -fsplit-machine-functions -S %s \
-// RUN:     2>&1 | FileCheck %s --check-prefix=MFS3
-// MFS3:     "-target-cpu" "x86-64"{{.*}}"-fsplit-machine-functions"
-// MFS3-NOT: "-target-cpu" "sm_70"{{.*}}"-fsplit-machine-functions"
-
-// Check that -Xarch_host -fsplit-machine-functions does not cause any warning.
-// RUN:   cd "$(dirname "%t")" ; \
-// RUN:   %clang --target=x86_64-unknown-linux-gnu -nogpulib -nogpuinc \
-// RUN      --cuda-gpu-arch=sm_70 -x cuda -Xarch_host \
-// RUN      -fsplit-machine-functions -S %s || { echo \
-// RUN      "warning: -fsplit-machine-functions is not valid for" ; } \
-// RUN      2>&1 | FileCheck %s --check-prefix=MFS4
-// MFS4-NOT: warning: -fsplit-machine-functions is not valid for
-
-// Check that -Xarch_device -fsplit-machine-functions does cause the warning.
-// RUN:   cd "$(dirname "%t")" ; \
-// RUN:   %clang --target=x86_64-unknown-linux-gnu -nogpulib -nogpuinc \
-// RUN:     --cuda-gpu-arch=sm_70 -x cuda -Xarch_device \
-// RUN:     -fsplit-machine-functions -S %s 2>&1 | \
-// RUN:     FileCheck %s --check-prefix=MFS5
-// MFS5: warning: -fsplit-machine-functions is not valid for
+// RUN:     2>&1 | FileCheck %s --check-prefix=MFS2
+// MFS2:     "-target-cpu" "x86-64"{{.*}}"-fsplit-machine-functions"
+// MFS2-NOT: "-target-cpu" "sm_70"{{.*}}"-fsplit-machine-functions"
 
 // Check that -fsplit-machine-functions -Xarch_device
 // -fno-split-machine-functions only passes MFS to x86
-// RUN:   cd "$(dirname "%t")" ; \
-// RUN:   %clang -### --target=x86_64-unknown-linux-gnu -nogpulib -nogpuinc \
+// RUN: %clang -### --target=x86_64-unknown-linux-gnu -nogpulib -nogpuinc \
 // RUN:     --cuda-gpu-arch=sm_70 -x cuda -fsplit-machine-functions \
 // RUN:     -Xarch_device -fno-split-machine-functions -S %s \
-// RUN:     2>&1 | FileCheck %s --check-prefix=MFS6
-// MFS6:     "-target-cpu" "x86-64"{{.*}}"-fsplit-machine-functions"
-// MFS6-NOT: "-target-cpu" "sm_70"{{.*}}"-fsplit-machine-functions"
-
-// Check that -fsplit-machine-functions -Xarch_device
-// -fno-split-machine-functions has no warnings
-// RUN:   cd "$(dirname "%t")" ; \
-// RUN:   %clang --target=x86_64-unknown-linux-gnu -nogpulib -nogpuinc \
-// RUN:     --cuda-gpu-arch=sm_70 -x cuda -fsplit-machine-functions \
-// RUN:     -Xarch_device -fno-split-machine-functions -S %s \
-// RUN:     || { echo "warning: -fsplit-machine-functions is not valid for"; } \
-// RUN:     2>&1 | FileCheck %s --check-prefix=MFS7
-// MFS7-NOT: warning: -fsplit-machine-functions is not valid for
+// RUN:     2>&1 | FileCheck %s --check-prefix=MFS3
+// MFS3:     "-target-cpu" "x86-64"{{.*}}"-fsplit-machine-functions"
+// MFS3-NOT: "-target-cpu" "sm_70"{{.*}}"-fsplit-machine-functions"

clang/test/Driver/fsplit-machine-functions.c

-// REQUIRES: arm-registered-target
+// RUN: %clang -### --target=x86_64 -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK_OPT %s
+// RUN: %clang -### --target=x86_64 -fprofile-use=default.profdata -fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK_NOOPT %s
 
-// RUN: %clang -### -target x86_64 -fprofile-use=default.profdata -fsplit-machine-functions %s -c -o %t.o 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
-// RUN: %clang -### -target x86_64 -fsplit-machine-functions %s -c -o %t.o 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
-// RUN: %clang -### -target x86_64 -fprofile-use=default.profdata -fsplit-machine-functions -fno-split-machine-functions %s -c -o %t.o 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
-// RUN: %clang -c -target arm-unknown-linux-gnueabi -fsplit-machine-functions %s -o %t.o 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
-
-// CHECK-OPT:       "-fsplit-machine-functions"
-// CHECK-NOOPT-NOT: "-fsplit-machine-functions"
-// CHECK-TRIPLE:    warning: -fsplit-machine-functions is not valid for arm
+// CHECK_OPT:        "-fsplit-machine-functions"
+// CHECK_NOOPT-NOT:  "-fsplit-machine-functions"

llvm/test/CodeGen/Generic/machine-function-splitter.ll

 ; RUN: sed 's/InstrProf/SampleProfile/g' %s > %t.ll
 ; RUN: llc < %t.ll -mtriple=x86_64-unknown-linux-gnu -split-machine-functions | FileCheck %s --check-prefix=FSAFDO-MFS
 ; RUN: llc < %t.ll -mtriple=x86_64-unknown-linux-gnu -split-machine-functions | FileCheck %s --check-prefix=FSAFDO-MFS2
 ; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -debug-pass=Structure -fs-profile-file=%S/Inputs/fsloader-mfs.afdo -enable-fs-discriminator=true -improved-fs-discriminator=true -split-machine-functions 2>&1 | FileCheck %s --check-prefix=MFS_ON
 ; RUN: llc < %s -mtriple=aarch64-unknown-linux-gnu -debug-pass=Structure -fs-profile-file=%S/Inputs/fsloader-mfs.afdo -enable-fs-discriminator=true -improved-fs-discriminator=true -split-machine-functions 2>&1 | FileCheck %s --check-prefix=MFS_OFF
 
 ;; Check that MFS is on for X86 targets.
 ; MFS_ON: Machine Function Splitter Transformation
-; MFS_ON_NO: warning: -fsplit-machine-functions is not valid for
+; MFS_ON-NOT: warning:
 ;; Check that MFS is not on for non-X86 targets.
 ; MFS_OFF: warning: -fsplit-machine-functions is not valid for
-; MFS_OFF_NO: Machine Function Splitter Transformation
+; MFS_OFF-NOT: Machine Function Splitter Transformation
 
 define void @foo1(i1 zeroext %0) nounwind !prof !14 !section_prefix !15 {
 ;; Check that cold block is moved to .text.split.
 ; MFS-DEFAULTS-LABEL:         foo1
 ; MFS-DEFAULTS:               .section        .text.split.foo1
 ; MFS-DEFAULTS-NEXT:          foo1.cold:
 ; MFS-DEFAULTS-X86-NOT:       callq   bar
 ; MFS-DEFAULTS-X86-NEXT:      callq   baz