[AMDGPU] Add code model (#70760) test for amdgpu target. #71019

pravinjagtap · 2023-11-02T05:12:59Z

Missed adding code model test during #70760.

llvmbot · 2023-11-02T05:13:30Z

@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-backend-amdgpu

Author: Pravin Jagtap (pravinjagtap)

Changes

Missed adding code model test during #70760.

Full diff: https://github.com/llvm/llvm-project/pull/71019.diff

1 Files Affected:

(added) llvm/test/CodeGen/AMDGPU/codemodel.ll (+9)

diff --git a/llvm/test/CodeGen/AMDGPU/codemodel.ll b/llvm/test/CodeGen/AMDGPU/codemodel.ll
new file mode 100644
index 000000000000000..8b60257b2076aed
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/codemodel.ll
@@ -0,0 +1,9 @@
+; RUN: not llc -verify-machineinstrs -o - -mtriple=amdgcn-unknown-amdhsa -mcpu=gfx900 -code-model=tiny < %s 2>&1 | FileCheck %s --check-prefix=TINY
+; RUN: not llc -verify-machineinstrs -o - -mtriple=amdgcn-unknown-amdhsa -mcpu=gfx900 -code-model=kernel < %s 2>&1 | FileCheck %s --check-prefix=KERNEL
+
+; TINY:    Target does not support the tiny CodeModel
+; KERNEL:    Target does not support the kernel CodeModel
+
+define void @foo() {
+  ret void
+}

arsenm

This doesn't test the driver behavior, which is what the patch changed. This should be a clang driver test, not codegen

arsenm · 2023-11-02T09:54:13Z

clang/test/Driver/mcmodel.c

@@ -13,6 +13,11 @@
 // RUN: not %clang -### -c --target=aarch64 -mcmodel=medium %s 2>&1 | FileCheck --check-prefix=ERR-MEDIUM %s
 // RUN: not %clang -### -c --target=aarch64 -mcmodel=kernel %s 2>&1 | FileCheck --check-prefix=ERR-KERNEL %s
 // RUN: not %clang --target=aarch64_32-linux -### -S -mcmodel=small %s 2>&1 | FileCheck --check-prefix=ERR-AARCH64_32 %s
+// RUN: %clang --target=amdgcn-amd-amdhsa -nogpulib -### -c -mcmodel=tiny %s 2>&1 | FileCheck --check-prefix=AMDGPU-MCMODEL-TINY %s


I'm not sure exactly what's being tested here, this looks like it's targeting C as a target? The more relevant case would be with amdgcn as the offload target. For direct compilation it would make more sense to emit the error

The change is intended to handle all possible code models for AMDGPU target. Basically, we want to avoid erroring out for device side code models. Does it make sense to have negative case for amdgpu ?

There's 2 cases. The case where we're directly codegening to the target, in which case it behaves like a regular CPU target. That's what you're testing here, but it's somewhat unusual. We could error here as before.

The more realistic case is treating it as an offload target from OpenMP or HIP, so I think it's more important to test that case. We definitely don't want an error here.

I think, recent updates to PR (3rd commit: 9dc7689983b74dd381df556bc9e61165e08d85b4) address both the cases you are referring to.

yxsamliu · 2023-11-02T15:37:59Z

llvm/test/CodeGen/AMDGPU/codemodel.ll

@@ -0,0 +1,9 @@
+; RUN: not llc -verify-machineinstrs -o - -mtriple=amdgcn-unknown-amdhsa -mcpu=gfx900 -code-model=tiny < %s 2>&1 | FileCheck %s --check-prefix=TINY
+; RUN: not llc -verify-machineinstrs -o - -mtriple=amdgcn-unknown-amdhsa -mcpu=gfx900 -code-model=kernel < %s 2>&1 | FileCheck %s --check-prefix=KERNEL


need test for all -code-model values

As matt suggested, this test is not exercising the code changes that I intended in #70760. So I will remove this test.

yxsamliu · 2023-11-06T14:00:18Z

I think we need to fix clang and not passing -mcmodel=tiny/kernel to clang -cc1.

https://github.com/llvm/llvm-project/pull/70760/files#r1383372722

probably we can do that in this PR

github-actions · 2023-11-07T09:08:39Z

✅ With the latest revision this PR passed the C/C++ code formatter.

arsenm · 2023-11-29T12:44:47Z

clang/lib/Driver/ToolChains/Clang.cpp

+    } else if (Triple.isAMDGPU()) {
+      // AMDGPU does not care about the code model.
+      Ok = true;
+      // AMDGPU target ignores CM tiny and kernel.
+      if (CM == "tiny" || CM == "kernel") {
+        Skip = true;
+        D.Diag(diag::warn_ignored_clang_option)
+            << A->getSpelling() << CM << TripleStr;
+      }


This is doing more than adding the test?

yxsamliu

LGTM. Please update PR title before merging

arsenm · 2024-02-01T09:58:09Z

LGTM. Please update PR title before merging

So this was only supposed to add the test, or implement this too?

arsenm · 2024-02-29T11:47:29Z

reverse ping

pravinjagtap requested review from arsenm, yxsamliu, aeubanks and jhuber6 November 2, 2023 05:12

llvmbot added the backend:AMDGPU label Nov 2, 2023

arsenm requested changes Nov 2, 2023

View reviewed changes

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Nov 2, 2023

arsenm reviewed Nov 2, 2023

View reviewed changes

yxsamliu reviewed Nov 2, 2023

View reviewed changes

pravinjagtap force-pushed the test-mcmode-amdgpu branch from 746c515 to d171525 Compare November 7, 2023 09:01

pravinjagtap force-pushed the test-mcmode-amdgpu branch 2 times, most recently from 7b3d4dc to 30f98ec Compare November 16, 2023 06:55

pravinjagtap requested review from yxsamliu and arsenm November 29, 2023 05:46

pravinjagtap added 6 commits November 29, 2023 00:55

[AMDGPU] Add code model (llvm#70760) test for amdgpu target.

2477ae8

Added clang driver test for code model.

4b3cf8a

Updated the mcodel test to target hip

7def0ff

Warn about CM tiny and kernel

f704e92

clang-format

fca9ce5

Handled skipping of tiny and kernel CM + rebase + clang-format

34c14bb

pravinjagtap force-pushed the test-mcmode-amdgpu branch from 30f98ec to 34c14bb Compare November 29, 2023 05:56

arsenm reviewed Nov 29, 2023

View reviewed changes

yxsamliu approved these changes Nov 29, 2023

View reviewed changes

Merge branch 'main' into test-mcmode-amdgpu

58c85c1

pravinjagtap closed this Mar 1, 2024

pravinjagtap deleted the test-mcmode-amdgpu branch March 1, 2024 04:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[AMDGPU] Add code model (#70760) test for amdgpu target. #71019

[AMDGPU] Add code model (#70760) test for amdgpu target. #71019

Uh oh!

pravinjagtap commented Nov 2, 2023

Uh oh!

llvmbot commented Nov 2, 2023 •

edited

Loading

Uh oh!

arsenm left a comment

Uh oh!

arsenm Nov 2, 2023

Uh oh!

pravinjagtap Nov 2, 2023

Uh oh!

arsenm Nov 3, 2023

Uh oh!

pravinjagtap Nov 3, 2023 •

edited

Loading

Uh oh!

yxsamliu Nov 2, 2023

Uh oh!

pravinjagtap Nov 3, 2023

Uh oh!

yxsamliu commented Nov 6, 2023

Uh oh!

github-actions bot commented Nov 7, 2023 •

edited

Loading

Uh oh!

arsenm Nov 29, 2023

Uh oh!

yxsamliu left a comment

Uh oh!

arsenm commented Feb 1, 2024

Uh oh!

arsenm commented Feb 29, 2024

Uh oh!

Uh oh!

		@@ -0,0 +1,9 @@
		; RUN: not llc -verify-machineinstrs -o - -mtriple=amdgcn-unknown-amdhsa -mcpu=gfx900 -code-model=tiny < %s 2>&1 \| FileCheck %s --check-prefix=TINY
		; RUN: not llc -verify-machineinstrs -o - -mtriple=amdgcn-unknown-amdhsa -mcpu=gfx900 -code-model=kernel < %s 2>&1 \| FileCheck %s --check-prefix=KERNEL

[AMDGPU] Add code model (#70760) test for amdgpu target. #71019

[AMDGPU] Add code model (#70760) test for amdgpu target. #71019

Uh oh!

Conversation

pravinjagtap commented Nov 2, 2023

Uh oh!

llvmbot commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

arsenm Nov 2, 2023

Choose a reason for hiding this comment

Uh oh!

pravinjagtap Nov 2, 2023

Choose a reason for hiding this comment

Uh oh!

arsenm Nov 3, 2023

Choose a reason for hiding this comment

Uh oh!

pravinjagtap Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yxsamliu Nov 2, 2023

Choose a reason for hiding this comment

Uh oh!

pravinjagtap Nov 3, 2023

Choose a reason for hiding this comment

Uh oh!

yxsamliu commented Nov 6, 2023

Uh oh!

github-actions bot commented Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arsenm Nov 29, 2023

Choose a reason for hiding this comment

Uh oh!

yxsamliu left a comment

Choose a reason for hiding this comment

Uh oh!

arsenm commented Feb 1, 2024

Uh oh!

arsenm commented Feb 29, 2024

Uh oh!

Uh oh!

llvmbot commented Nov 2, 2023 •

edited

Loading

pravinjagtap Nov 3, 2023 •

edited

Loading

github-actions bot commented Nov 7, 2023 •

edited

Loading