[Driver] Report invalid target triple versions for all environment types. #78655

ZijunZhaoCCK · 2024-01-19T00:51:53Z

Followup for #75373

Make this feature not just available for android, but everyone.
Correct some target triples.
Add opencl to the environment type list.

Followup for llvm#75373 1. Make this feature not just available for android, but everyone. 2. Correct some target triples/ 3. Add opencl to the environment type list.

llvmbot · 2024-01-19T00:52:23Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: None (ZijunZhaoCCK)

Changes

Followup for #75373

Make this feature not just available for android, but everyone.
Correct some target triples/
Add opencl to the environment type list.

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

6 Files Affected:

(modified) clang/lib/Driver/Driver.cpp (+10-9)
(modified) clang/test/CodeGen/fp128_complex.c (+1-1)
(modified) clang/test/Driver/mips-features.c (+2-2)
(modified) clang/test/Frontend/fixed_point_bit_widths.c (+2-2)
(modified) llvm/include/llvm/TargetParser/Triple.h (+1-1)
(modified) llvm/lib/TargetParser/Triple.cpp (+14)

diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 1889ea28079df10..2d6986d145483b8 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -1430,15 +1430,16 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
   const ToolChain &TC = getToolChain(
       *UArgs, computeTargetTriple(*this, TargetTriple, *UArgs));
 
-  if (TC.getTriple().isAndroid()) {
-    llvm::Triple Triple = TC.getTriple();
-    StringRef TripleVersionName = Triple.getEnvironmentVersionString();
-
-    if (Triple.getEnvironmentVersion().empty() && TripleVersionName != "") {
-      Diags.Report(diag::err_drv_triple_version_invalid)
-          << TripleVersionName << TC.getTripleString();
-      ContainsError = true;
-    }
+  // Check if the environment version is valid.
+  llvm::Triple Triple = TC.getTriple();
+  StringRef TripleVersionName = Triple.getEnvironmentVersionString();
+  StringRef TripleObjectFormat = Triple.getObjectFormatTypeName(Triple.getObjectFormat());
+
+  if (Triple.getEnvironmentVersion().empty() && TripleVersionName != ""
+    && TripleVersionName != TripleObjectFormat) {
+    Diags.Report(diag::err_drv_triple_version_invalid)
+        << TripleVersionName << TC.getTripleString();
+    ContainsError = true;
   }
 
   // Report warning when arm64EC option is overridden by specified target
diff --git a/clang/test/CodeGen/fp128_complex.c b/clang/test/CodeGen/fp128_complex.c
index 48659d224168416..0e87cbe8ce81219 100644
--- a/clang/test/CodeGen/fp128_complex.c
+++ b/clang/test/CodeGen/fp128_complex.c
@@ -1,4 +1,4 @@
-// RUN: %clang -target aarch64-linux-gnuabi %s -S -emit-llvm -o - | FileCheck %s
+// RUN: %clang -target aarch64-linux-gnueabi %s -S -emit-llvm -o - | FileCheck %s
 
 _Complex long double a, b, c, d;
 void test_fp128_compound_assign(void) {
diff --git a/clang/test/Driver/mips-features.c b/clang/test/Driver/mips-features.c
index fad6009ffb89bab..18edcd05ea85c9f 100644
--- a/clang/test/Driver/mips-features.c
+++ b/clang/test/Driver/mips-features.c
@@ -400,12 +400,12 @@
 // LONG-CALLS-DEF-NOT: "long-calls"
 //
 // -mbranch-likely
-// RUN: %clang -target -mips-mti-linux-gnu -### -c %s -mbranch-likely 2>&1 \
+// RUN: %clang -target mips-mti-linux-gnu -### -c %s -mbranch-likely 2>&1 \
 // RUN:   | FileCheck --check-prefix=BRANCH-LIKELY %s
 // BRANCH-LIKELY: argument unused during compilation: '-mbranch-likely'
 //
 // -mno-branch-likely
-// RUN: %clang -target -mips-mti-linux-gnu -### -c %s -mno-branch-likely 2>&1 \
+// RUN: %clang -target mips-mti-linux-gnu -### -c %s -mno-branch-likely 2>&1 \
 // RUN:   | FileCheck --check-prefix=NO-BRANCH-LIKELY %s
 // NO-BRANCH-LIKELY: argument unused during compilation: '-mno-branch-likely'
 
diff --git a/clang/test/Frontend/fixed_point_bit_widths.c b/clang/test/Frontend/fixed_point_bit_widths.c
index ac8db49ed516aef..e56f787e824f24a 100644
--- a/clang/test/Frontend/fixed_point_bit_widths.c
+++ b/clang/test/Frontend/fixed_point_bit_widths.c
@@ -1,7 +1,7 @@
 // RUN: %clang -x c -ffixed-point -S -emit-llvm -o - %s | FileCheck %s
-// RUN: %clang -x c -ffixed-point -S -emit-llvm -o - --target=x86_64-scei-ps4-ubuntu-fast %s | FileCheck %s
+// RUN: %clang -x c -ffixed-point -S -emit-llvm -o - --target=x86_64-scei-ps4 %s | FileCheck %s
 // RUN: %clang -x c -ffixed-point -S -emit-llvm -o - --target=ppc64 %s | FileCheck %s
-// RUN: %clang -x c -ffixed-point -S -emit-llvm -o - --target=x86_64-scei-ps4-windows10pro-fast %s | FileCheck %s
+// RUN: %clang -x c -ffixed-point -S -emit-llvm -o - --target=x86_64-scei-ps4 %s | FileCheck %s
 
 /* Primary signed _Accum */
 
diff --git a/llvm/include/llvm/TargetParser/Triple.h b/llvm/include/llvm/TargetParser/Triple.h
index 95014a546f72453..525ea6df3643ca6 100644
--- a/llvm/include/llvm/TargetParser/Triple.h
+++ b/llvm/include/llvm/TargetParser/Triple.h
@@ -273,7 +273,7 @@ class Triple {
     Callable,
     Mesh,
     Amplification,
-
+    OpenCL,
     OpenHOS,
 
     LastEnvironmentType = OpenHOS
diff --git a/llvm/lib/TargetParser/Triple.cpp b/llvm/lib/TargetParser/Triple.cpp
index b9971c25af71f39..7bb2fb9d1365e2f 100644
--- a/llvm/lib/TargetParser/Triple.cpp
+++ b/llvm/lib/TargetParser/Triple.cpp
@@ -320,6 +320,7 @@ StringRef Triple::getEnvironmentTypeName(EnvironmentType Kind) {
   case Callable: return "callable";
   case Mesh: return "mesh";
   case Amplification: return "amplification";
+  case OpenCL: return "opencl";
   case OpenHOS: return "ohos";
   }
 
@@ -687,6 +688,7 @@ static Triple::EnvironmentType parseEnvironment(StringRef EnvironmentName) {
       .StartsWith("callable", Triple::Callable)
       .StartsWith("mesh", Triple::Mesh)
       .StartsWith("amplification", Triple::Amplification)
+      .StartsWith("opencl", Triple::OpenCL)
       .StartsWith("ohos", Triple::OpenHOS)
       .Default(Triple::UnknownEnvironment);
 }
@@ -1211,8 +1213,20 @@ VersionTuple Triple::getEnvironmentVersion() const {
 
 StringRef Triple::getEnvironmentVersionString() const {
   StringRef EnvironmentName = getEnvironmentName();
+
+  // none is a valid environment type - it basically amounts to a freestanding environment.
+  if (EnvironmentName == "none")
+    return "";
+
   StringRef EnvironmentTypeName = getEnvironmentTypeName(getEnvironment());
   EnvironmentName.consume_front(EnvironmentTypeName);
+
+  if (EnvironmentName.starts_with("-")) {
+    // arch-vendor-os-env-obj is correct
+    StringRef ObjectFormatType = getObjectFormatTypeName(getObjectFormat());
+    if (ObjectFormatType == StringRef(EnvironmentName).split('-').second)
+      return "";
+  }
   return EnvironmentName;
 }

github-actions · 2024-01-19T00:54:19Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 7a94acb2da5b20d12f13f3c5f4eb0f3f46e78e73 6f41d09f2265338b6403cbb345b7befdb84fe115 -- clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-char.cpp clang/lib/Driver/Driver.cpp clang/test/CodeGen/fp128_complex.c clang/test/Driver/mips-features.c clang/test/Frontend/fixed_point_bit_widths.c llvm/include/llvm/TargetParser/Triple.h llvm/lib/TargetParser/Triple.cpp

View the diff from clang-format here.

diff --git a/llvm/include/llvm/TargetParser/Triple.h b/llvm/include/llvm/TargetParser/Triple.h
index 98d8490cc9..5683afd583 100644
--- a/llvm/include/llvm/TargetParser/Triple.h
+++ b/llvm/include/llvm/TargetParser/Triple.h
@@ -255,7 +255,7 @@ public:
     Cygnus,
     CoreCLR,
     Simulator, // Simulator variants of other systems, e.g., Apple's iOS
-    MacABI, // Mac Catalyst variant of Apple's iOS deployment target.
+    MacABI,    // Mac Catalyst variant of Apple's iOS deployment target.
 
     // Shader Stages
     // The order of these values matters, and must be kept in sync with the

clang/test/Frontend/fixed_point_bit_widths.c

Followup for llvm#75373 1. Make this feature not just available for android, but everyone. 2. Correct some target triples. 3. Add opencl to the environment type list.

pirama-arumuga-nainar · 2024-01-25T20:09:53Z

@llvm/clang-vendors Adding clang vendors. FYI, this change expands error reporting on invalid version numbers to all target triples (This was previously restricted to Android triples). This can have potential downstream impact. Please review/test and let us know if this breaks any valid usages.

clang/lib/Driver/Driver.cpp

llvm/include/llvm/TargetParser/Triple.h

Followup for llvm#75373 1. Make this feature not just available for android, but everyone. 2. Correct some target triples. 3. Add opencl to the environment type list.

llvm/lib/TargetParser/Triple.cpp

clang/test/Driver/mips-features.c

Followup for llvm#75373 1. Make this feature not just available for android, but everyone. 2. Correct some target triples. 3. Add opencl to the environment type list.

MaskRay

A better title may be:

[Driver] Report invalid target triple versions for all environment types.

"Driver" is better than "clang" as it is specific about where the error arises.

llvm/lib/TargetParser/Triple.cpp

Followup for llvm#75373 1. Make this feature not just available for android, but everyone. 2. Correct some target triples. 3. Add opencl to the environment type list.

glandium · 2024-02-05T20:53:23Z

This broke the wasi-threads target:
clang: error: version 'threads' in target triple 'wasm32-unknown-wasi-threads' is invalid

…pes. (llvm#78655) Followup for llvm#75373 1. Make this feature not just available for android, but everyone. 2. Correct some target triples. 3. Add opencl to the environment type list.

ZijunZhaoCCK · 2024-02-05T23:00:09Z

This broke the wasi-threads target: clang: error: version 'threads' in target triple 'wasm32-unknown-wasi-threads' is invalid

Because threads is not in EnvironmentType list: https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/TargetParser/Triple.h#L231 or ObjectType list: https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/TargetParser/Triple.h#L284 .

The format should be arch-vendor-os-env. If threads is a new environment type, please add it in the list. If wasi-threads is a special case, please let me know!

And one more question, would you mind pointing me out the test case of wasm32-unknown-wasi-threads ? I don't see it. Thank you!

glandium · 2024-02-05T23:16:47Z

We stumbled upon this downstream because we have jobs building wasi-sdk with clang-trunk, and wasi-sdk builds some things with that target. It apparently comes from WebAssembly/wasi-libc#381

glandium · 2024-02-05T23:18:16Z

There's apparently also wasm32-wasi-preview2 and wasm32-wasi-pthread, which I suppose are equally broken by this change.

ZijunZhaoCCK · 2024-02-05T23:45:04Z

There's apparently also wasm32-wasi-preview2 and wasm32-wasi-pthread, which I suppose are equally broken by this change.

Yes, I think so. I think adding these environment types in wasi-libc repo could help fix those errors.

glandium · 2024-02-05T23:51:53Z

Yes, I think so. I think adding these environment types in wasi-libc repo could help fix those errors.

WDYM?

MaskRay · 2024-02-05T23:55:43Z

There's apparently also wasm32-wasi-preview2 and wasm32-wasi-pthread, which I suppose are equally broken by this change.

Yes, I think so. I think adding these environment types in wasi-libc repo could help fix those errors.

If wasm can arbitrary environment types, it seems that we can opt out the check for isWasm().
We define new environment types when they affect the compiler behavior. If wasm arbitrary sets the environment, we should not define a type for each one.

ZijunZhaoCCK · 2024-02-06T00:37:12Z

There's apparently also wasm32-wasi-preview2 and wasm32-wasi-pthread, which I suppose are equally broken by this change.

Yes, I think so. I think adding these environment types in wasi-libc repo could help fix those errors.

If wasm can arbitrary environment types, it seems that we can opt out the check for isWasm(). We define new environment types when they affect the compiler behavior. If wasm arbitrary sets the environment, we should not define a type for each one.

Okay, I will add this check.

Add isWasm() check for here: #78655 (comment)

This updates the HLSL invalid environment tests to adapt to llvm#78655.

Make clang report invalid target versions for all environment.

f440f44

Followup for llvm#75373 1. Make this feature not just available for android, but everyone. 2. Correct some target triples/ 3. Add opencl to the environment type list.

ZijunZhaoCCK requested review from bogner, MaskRay, llvm-beanz, pirama-arumuga-nainar and kazutakahirata January 19, 2024 00:51

ZijunZhaoCCK changed the title ~~Make clang report invalid target versions for all environment.~~ Make clang report invalid target versions for all environment types. Jan 19, 2024

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

Merge branch 'llvm:main' into check-version

4d0c8f6

MaskRay reviewed Jan 19, 2024

View reviewed changes

clang/test/Frontend/fixed_point_bit_widths.c Show resolved Hide resolved

ZijunZhaoCCK and others added 7 commits January 19, 2024 13:02

Merge branch 'llvm:main' into check-version

1f0a2ee

Merge branch 'llvm:main' into check-version

b5daff2

Make clang report invalid target versions for all environment.

a2e1335

Followup for llvm#75373 1. Make this feature not just available for android, but everyone. 2. Correct some target triples. 3. Add opencl to the environment type list.

Merge branch 'llvm:main' into check-version

a5052ba

Make clang report invalid target versions for all environment.

036ea34

Followup for llvm#75373 1. Make this feature not just available for android, but everyone. 2. Correct some target triples. 3. Add opencl to the environment type list.

Merge branch 'llvm:main' into check-version

c756121

Make clang report invalid target versions for all environment.

2ab3f7b

Followup for llvm#75373 1. Make this feature not just available for android, but everyone. 2. Correct some target triples. 3. Add opencl to the environment type list.

MaskRay reviewed Jan 25, 2024

View reviewed changes

clang/lib/Driver/Driver.cpp Outdated Show resolved Hide resolved

MaskRay reviewed Jan 25, 2024

View reviewed changes

llvm/include/llvm/TargetParser/Triple.h Outdated Show resolved Hide resolved

MaskRay reviewed Jan 25, 2024

View reviewed changes

llvm/include/llvm/TargetParser/Triple.h Show resolved Hide resolved

ZijunZhaoCCK and others added 4 commits January 25, 2024 18:06

Merge branch 'llvm:main' into check-version

30be503

Make clang report invalid target versions for all environment.

f32525a

Followup for llvm#75373 1. Make this feature not just available for android, but everyone. 2. Correct some target triples. 3. Add opencl to the environment type list.

Merge branch 'llvm:main' into check-version

c60ea16

Merge branch 'llvm:main' into check-version

d657b7f

MaskRay reviewed Feb 1, 2024

View reviewed changes

llvm/lib/TargetParser/Triple.cpp Outdated Show resolved Hide resolved

MaskRay reviewed Feb 1, 2024

View reviewed changes

clang/test/Driver/mips-features.c Outdated Show resolved Hide resolved

ZijunZhaoCCK and others added 2 commits February 1, 2024 15:45

Merge branch 'llvm:main' into check-version

6173010

Make clang report invalid target versions for all environment.

c016d09

Followup for llvm#75373 1. Make this feature not just available for android, but everyone. 2. Correct some target triples. 3. Add opencl to the environment type list.

MaskRay approved these changes Feb 2, 2024

View reviewed changes

llvm/lib/TargetParser/Triple.cpp Outdated Show resolved Hide resolved

ZijunZhaoCCK changed the title ~~Make clang report invalid target versions for all environment types.~~ [Driver] Report invalid target triple versions for all environment types. Feb 2, 2024

ZijunZhaoCCK and others added 2 commits February 2, 2024 13:42

Merge branch 'llvm:main' into check-version

3d25a65

Make clang report invalid target versions for all environment.

4458696

Followup for llvm#75373 1. Make this feature not just available for android, but everyone. 2. Correct some target triples. 3. Add opencl to the environment type list.

pirama-arumuga-nainar approved these changes Feb 2, 2024

View reviewed changes

ZijunZhaoCCK and others added 3 commits February 2, 2024 14:36

Merge branch 'llvm:main' into check-version

ee0355c

Merge branch 'llvm:main' into check-version

a2976a8

Make clang report invalid target versions for all environment.

6f41d09

Followup for llvm#75373 1. Make this feature not just available for android, but everyone. 2. Correct some target triples. 3. Add opencl to the environment type list.

ZijunZhaoCCK merged commit 34c4a0f into llvm:main Feb 5, 2024
3 of 4 checks passed

ZijunZhaoCCK deleted the check-version branch February 5, 2024 00:42

ZijunZhaoCCK mentioned this pull request Feb 6, 2024

[Driver] Check the environment version except wasm case. #80783

Merged

ZijunZhaoCCK added a commit that referenced this pull request Feb 6, 2024

[Driver] Check the environment version except wasm case. (#80783)

fbded66

Add isWasm() check for here: #78655 (comment)

llvm-beanz added a commit to llvm-beanz/llvm-project that referenced this pull request Feb 7, 2024

[NFC][HLSL][DX] Update invalid environment tests

7deda6c

This updates the HLSL invalid environment tests to adapt to llvm#78655.

llvm-beanz mentioned this pull request Feb 7, 2024

[NFC][HLSL][DX] Update invalid environment tests #81052

Closed

MaskRay added a commit that referenced this pull request Feb 12, 2024

[Driver,test] Fix hlsl-lang-targets.hlsl after #78655

fb48fd1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Driver] Report invalid target triple versions for all environment types. #78655

[Driver] Report invalid target triple versions for all environment types. #78655

ZijunZhaoCCK commented Jan 19, 2024 •

edited

llvmbot commented Jan 19, 2024 •

edited

github-actions bot commented Jan 19, 2024 •

edited

pirama-arumuga-nainar commented Jan 25, 2024

MaskRay left a comment

glandium commented Feb 5, 2024

ZijunZhaoCCK commented Feb 5, 2024

glandium commented Feb 5, 2024

glandium commented Feb 5, 2024

ZijunZhaoCCK commented Feb 5, 2024

glandium commented Feb 5, 2024

MaskRay commented Feb 5, 2024

ZijunZhaoCCK commented Feb 6, 2024

[Driver] Report invalid target triple versions for all environment types. #78655

[Driver] Report invalid target triple versions for all environment types. #78655

Conversation

ZijunZhaoCCK commented Jan 19, 2024 • edited

llvmbot commented Jan 19, 2024 • edited

github-actions bot commented Jan 19, 2024 • edited

pirama-arumuga-nainar commented Jan 25, 2024

MaskRay left a comment

Choose a reason for hiding this comment

glandium commented Feb 5, 2024

ZijunZhaoCCK commented Feb 5, 2024

glandium commented Feb 5, 2024

glandium commented Feb 5, 2024

ZijunZhaoCCK commented Feb 5, 2024

glandium commented Feb 5, 2024

MaskRay commented Feb 5, 2024

ZijunZhaoCCK commented Feb 6, 2024

ZijunZhaoCCK commented Jan 19, 2024 •

edited

llvmbot commented Jan 19, 2024 •

edited

github-actions bot commented Jan 19, 2024 •

edited