Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RISCV] Update TargetAttr target-cpu override rule #75804

Closed
wants to merge 2 commits into from

Conversation

BeMg
Copy link
Contributor

@BeMg BeMg commented Dec 18, 2023

Address question from #74889


The target-cpu will infer its target-feature during backend codegen, it will make TargetAttr doesn't work.

This patch implement the following override rule

by

  1. When implying Full-Arch-String in target attribute, replace target-cpu with baseline cpu to make sure backend doesn't infer other target feature.
  2. Keep mcpu in tune-cpu during Attr-cpu,Attr-tune not present.

Driver with march, mcpu

No. Attr-arch Attr-cpu Attr-tune target-cpu target-feature tune-cpu
1 No No No mcpu march No
2 Full-Arch-String No No generic-rv32/rv64 attr-arch mcpu
3 Full-Arch-String Yes No generic-rv32/rv64 attr-arch attr-cpu
4 Full-Arch-String No Yes generic-rv32/rv64 attr-arch attr-tune
5 Full-Arch-String Yes Yes generic-rv32/rv64 attr-arch attr-tune
6 Adding-Extension No No mcpu march + attr-arch No
7 Adding-Extension Yes No mcpu march + attr-arch attr-cpu
8 Adding-Extension No Yes mcpu march + attr-arch attr-tune
9 Adding-Extension Yes Yes mcpu march + attr-arch attr-tune
10 No Yes No attr-cpu attr-cpu No
11 No Yes Yes attr-cpu attr-cpu attr-tune
12 No No Yes mcpu march attr-tune

Note

This patch assumes that target-cpu is only used to infer target-feature and tune-cpu in the backend. If there are other purposes for target-cpu, then this patch need to find the alternative solution.

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:RISC-V clang:codegen labels Dec 18, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 18, 2023

@llvm/pr-subscribers-backend-risc-v
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Piyou Chen (BeMg)

Changes

Address question from #74889


The target-cpu will infer its target-feature during backend codegen, it will make TargetAttr doesn't work.

This patch implement the following override rule

by

  1. When implying Full-Arch-String in target attribute, replace target-cpu with baseline cpu to make sure backend doesn't infer other target feature.
  2. Keep mcpu in tune-cpu during Attr-cpu,Attr-tune not present.

Driver with march, mcpu

No. Attr-arch Attr-cpu Attr-tune target-cpu target-feature tune-cpu
1 No No No mcpu march No
2 Full-Arch-String No No generic-rv32/rv64 attr-arch mcpu
3 Full-Arch-String Yes No generic-rv32/rv64 attr-arch attr-cpu
4 Full-Arch-String No Yes generic-rv32/rv64 attr-arch attr-tune
5 Full-Arch-String Yes Yes generic-rv32/rv64 attr-arch attr-tune
6 Adding-Extension No No mcpu march + attr-arch No
7 Adding-Extension Yes No mcpu march + attr-arch attr-cpu
8 Adding-Extension No Yes mcpu march + attr-arch attr-tune
9 Adding-Extension Yes Yes mcpu march + attr-arch attr-tune
10 No Yes No attr-cpu attr-cpu No
11 No Yes Yes attr-cpu attr-cpu attr-tune
12 No No Yes mcpu march attr-tune

Note

This patch assumes that target-cpu is only used to infer target-feature and tune-cpu in the backend. If there are other purposes for target-cpu, then this patch need to find the alternative solution.


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

5 Files Affected:

  • (modified) clang/lib/CodeGen/CodeGenModule.cpp (+25)
  • (added) clang/test/CodeGen/RISCV/riscv-func-attr-target-mcpu-rv32.c (+63)
  • (added) clang/test/CodeGen/RISCV/riscv-func-attr-target-mcpu-rv64.c (+63)
  • (modified) clang/test/CodeGen/RISCV/riscv-func-attr-target.c (+5-5)
  • (added) llvm/test/CodeGen/RISCV/riscv-func-target-feature-mcpu-override.ll (+36)
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 7ad26ace328ab2..83a530f1fee45d 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -2571,6 +2571,31 @@ bool CodeGenModule::GetCPUAndFeaturesAttributes(GlobalDecl GD,
       if (!ParsedAttr.Tune.empty() &&
           getTarget().isValidCPUName(ParsedAttr.Tune))
         TuneCPU = ParsedAttr.Tune;
+
+      if (getTarget().getTriple().isRISCV()) {
+        // attr-cpu override march only if arch isn't present.
+        if (TD->getFeaturesStr().contains("arch=")) {
+          // Before drop attr-cpu, keep it in tune-cpu
+          if (!TargetCPU.empty() && TuneCPU.empty())
+            TuneCPU = TargetCPU;
+
+          // Reassign mcpu due to attr-arch=<Adding-Extension> need
+          // target-feature from mcpu/march.
+          // Use attr-cpu will affect target-feature.
+          TargetCPU = getTarget().getTargetOpts().CPU;
+
+          // arch=<full-arch-string> need keep target feature clean,
+          // use the baseline cpu.
+          if (llvm::find(ParsedAttr.Features,
+                         "__RISCV_TargetAttrNeedOverride") !=
+              ParsedAttr.Features.end())
+            TargetCPU = (getTarget().getTriple().isRISCV32()) ? "generic-rv32"
+                                                              : "generic-rv64";
+
+          if (TargetCPU == TuneCPU)
+            TuneCPU = "";
+        }
+      }
     }
 
     if (SD) {
diff --git a/clang/test/CodeGen/RISCV/riscv-func-attr-target-mcpu-rv32.c b/clang/test/CodeGen/RISCV/riscv-func-attr-target-mcpu-rv32.c
new file mode 100644
index 00000000000000..abbeb57f54faa3
--- /dev/null
+++ b/clang/test/CodeGen/RISCV/riscv-func-attr-target-mcpu-rv32.c
@@ -0,0 +1,63 @@
+// REQUIRES: riscv-registered-target
+// RUN: %clang_cc1 -triple riscv32 -target-cpu sifive-e76 -target-feature +zifencei -target-feature +m \
+// RUN:  -target-feature +a -target-feature +save-restore -target-feature -zbb \
+// RUN:  -target-feature -relax -target-feature -zfa \
+// RUN:  -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: define dso_local void @testDefault() #0
+void testDefault() {}
+
+// CHECK: define dso_local void @testFullArchOnly() #1
+__attribute__((target("arch=rv32imac"))) void
+testFullArchOnly() {}
+
+// CHECK: define dso_local void @testFullArchAndCpu() #2
+__attribute__((target("arch=rv32imac;cpu=sifive-e34"))) void
+testFullArchAndCpu() {}
+
+// CHECK: define dso_local void @testFullArchAndTune() #2
+__attribute__((target("arch=rv32imac;tune=sifive-e34"))) void
+testFullArchAndTune() {}
+
+// CHECK: define dso_local void @testFullArchAndCpuAndTune() #2
+__attribute__((target("arch=rv32imac;cpu=sifive-e31;tune=sifive-e34"))) void
+testFullArchAndCpuAndTune() {}
+
+// CHECK: define dso_local void @testAddExtOnly() #3
+__attribute__((target("arch=+v"))) void
+testAddExtOnly() {}
+
+// CHECK: define dso_local void @testAddExtAndCpu() #4
+__attribute__((target("arch=+v;cpu=sifive-e31"))) void
+testAddExtAndCpu() {}
+
+// CHECK: define dso_local void @testAddExtAndTune() #4
+__attribute__((target("arch=+v;tune=sifive-e31"))) void
+testAddExtAndTune() {}
+
+// CHECK: define dso_local void @testAddExtAndCpuAndTune() #5
+__attribute__((target("arch=+v;cpu=sifive-e31;tune=sifive-e34"))) void
+testAddExtAndCpuAndTune() {}
+
+// CHECK: define dso_local void @testCpuOnly() #6
+__attribute__((target("cpu=sifive-e31"))) void
+testCpuOnly() {}
+
+// CHECK: define dso_local void @testCpuAndTune() #7
+__attribute__((target("cpu=sifive-e31;tune=sifive-e34"))) void
+testCpuAndTune() {}
+
+// CHECK: define dso_local void @testTuneOnly() #8
+__attribute__((target("tune=sifive-e34"))) void
+testTuneOnly() {}
+
+// .
+// CHECK: attributes #0 = { {{.*}}"target-cpu"="sifive-e76" "target-features"="+32bit,+a,+m,+save-restore,+zifencei,-relax,-zbb,-zfa" }
+// CHECK: attributes #1 = { {{.*}}"target-cpu"="generic-rv32" "target-features"="+32bit,+a,+c,+m,+save-restore,-relax,-zbb,-zfa" "tune-cpu"="sifive-e76" }
+// CHECK: attributes #2 = { {{.*}}"target-cpu"="generic-rv32" "target-features"="+32bit,+a,+c,+m,+save-restore,-relax,-zbb,-zfa" "tune-cpu"="sifive-e34" }
+// CHECK: attributes #3 = { {{.*}}"target-cpu"="sifive-e76" "target-features"="+32bit,+a,+d,+f,+m,+save-restore,+v,+zicsr,+zifencei,+zve32f,+zve32x,+zve64d,+zve64f,+zve64x,+zvl128b,+zvl32b,+zvl64b,-relax,-zbb,-zfa" }
+// CHECK: attributes #4 = { {{.*}}"target-cpu"="sifive-e76" "target-features"="+32bit,+a,+d,+f,+m,+save-restore,+v,+zicsr,+zifencei,+zve32f,+zve32x,+zve64d,+zve64f,+zve64x,+zvl128b,+zvl32b,+zvl64b,-relax,-zbb,-zfa" "tune-cpu"="sifive-e31" }
+// CHECK: attributes #5 = { {{.*}}"target-cpu"="sifive-e76" "target-features"="+32bit,+a,+d,+f,+m,+save-restore,+v,+zicsr,+zifencei,+zve32f,+zve32x,+zve64d,+zve64f,+zve64x,+zvl128b,+zvl32b,+zvl64b,-relax,-zbb,-zfa" "tune-cpu"="sifive-e34" }
+// CHECK: attributes #6 = { {{.*}}"target-cpu"="sifive-e31" "target-features"="+32bit,+a,+c,+m,+save-restore,+zicsr,+zifencei,-relax,-zbb,-zfa" }
+// CHECK: attributes #7 = { {{.*}}"target-cpu"="sifive-e31" "target-features"="+32bit,+a,+c,+m,+save-restore,+zicsr,+zifencei,-relax,-zbb,-zfa" "tune-cpu"="sifive-e34" }
+// CHECK: attributes #8 = { {{.*}}"target-cpu"="sifive-e76" "target-features"="+32bit,+a,+m,+save-restore,+zifencei,-relax,-zbb,-zfa" "tune-cpu"="sifive-e34" }
diff --git a/clang/test/CodeGen/RISCV/riscv-func-attr-target-mcpu-rv64.c b/clang/test/CodeGen/RISCV/riscv-func-attr-target-mcpu-rv64.c
new file mode 100644
index 00000000000000..9cb12e85e84215
--- /dev/null
+++ b/clang/test/CodeGen/RISCV/riscv-func-attr-target-mcpu-rv64.c
@@ -0,0 +1,63 @@
+// REQUIRES: riscv-registered-target
+// RUN: %clang_cc1 -triple riscv64 -target-cpu sifive-x280 -target-feature +zifencei -target-feature +m \
+// RUN:  -target-feature +a -target-feature +save-restore -target-feature -zbb \
+// RUN:  -target-feature -relax -target-feature -zfa \
+// RUN:  -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: define dso_local void @testDefault() #0
+void testDefault() {}
+
+// CHECK: define dso_local void @testFullArchOnly() #1
+__attribute__((target("arch=rv64imac"))) void
+testFullArchOnly() {}
+
+// CHECK: define dso_local void @testFullArchAndCpu() #2
+__attribute__((target("arch=rv64imac;cpu=sifive-u74"))) void
+testFullArchAndCpu() {}
+
+// CHECK: define dso_local void @testFullArchAndTune() #2
+__attribute__((target("arch=rv64imac;tune=sifive-u74"))) void
+testFullArchAndTune() {}
+
+// CHECK: define dso_local void @testFullArchAndCpuAndTune() #2
+__attribute__((target("arch=rv64imac;cpu=sifive-u54;tune=sifive-u74"))) void
+testFullArchAndCpuAndTune() {}
+
+// CHECK: define dso_local void @testAddExtOnly() #3
+__attribute__((target("arch=+v"))) void
+testAddExtOnly() {}
+
+// CHECK: define dso_local void @testAddExtAndCpu() #4
+__attribute__((target("arch=+v;cpu=sifive-u54"))) void
+testAddExtAndCpu() {}
+
+// CHECK: define dso_local void @testAddExtAndTune() #4
+__attribute__((target("arch=+v;tune=sifive-u54"))) void
+testAddExtAndTune() {}
+
+// CHECK: define dso_local void @testAddExtAndCpuAndTune() #5
+__attribute__((target("arch=+v;cpu=sifive-u54;tune=sifive-u74"))) void
+testAddExtAndCpuAndTune() {}
+
+// CHECK: define dso_local void @testCpuOnly() #6
+__attribute__((target("cpu=sifive-u54"))) void
+testCpuOnly() {}
+
+// CHECK: define dso_local void @testCpuAndTune() #7
+__attribute__((target("cpu=sifive-u54;tune=sifive-u74"))) void
+testCpuAndTune() {}
+
+// CHECK: define dso_local void @testTuneOnly() #8
+__attribute__((target("tune=sifive-u74"))) void
+testTuneOnly() {}
+
+// .
+// CHECK: attributes #0 = { {{.*}}"target-cpu"="sifive-x280" "target-features"="+64bit,+a,+m,+save-restore,+zifencei,-relax,-zbb,-zfa" }
+// CHECK: attributes #1 = { {{.*}}"target-cpu"="generic-rv64" "target-features"="+64bit,+a,+c,+m,+save-restore,-relax,-zbb,-zfa" "tune-cpu"="sifive-x280" }
+// CHECK: attributes #2 = { {{.*}}"target-cpu"="generic-rv64" "target-features"="+64bit,+a,+c,+m,+save-restore,-relax,-zbb,-zfa" "tune-cpu"="sifive-u74" }
+// CHECK: attributes #3 = { {{.*}}"target-cpu"="sifive-x280" "target-features"="+64bit,+a,+d,+f,+m,+save-restore,+v,+zicsr,+zifencei,+zve32f,+zve32x,+zve64d,+zve64f,+zve64x,+zvl128b,+zvl32b,+zvl64b,-relax,-zbb,-zfa" }
+// CHECK: attributes #4 = { {{.*}}"target-cpu"="sifive-x280" "target-features"="+64bit,+a,+d,+f,+m,+save-restore,+v,+zicsr,+zifencei,+zve32f,+zve32x,+zve64d,+zve64f,+zve64x,+zvl128b,+zvl32b,+zvl64b,-relax,-zbb,-zfa" "tune-cpu"="sifive-u54" }
+// CHECK: attributes #5 = { {{.*}}"target-cpu"="sifive-x280" "target-features"="+64bit,+a,+d,+f,+m,+save-restore,+v,+zicsr,+zifencei,+zve32f,+zve32x,+zve64d,+zve64f,+zve64x,+zvl128b,+zvl32b,+zvl64b,-relax,-zbb,-zfa" "tune-cpu"="sifive-u74" }
+// CHECK: attributes #6 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+a,+c,+d,+f,+m,+save-restore,+zicsr,+zifencei,-relax,-zbb,-zfa" }
+// CHECK: attributes #7 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+a,+c,+d,+f,+m,+save-restore,+zicsr,+zifencei,-relax,-zbb,-zfa" "tune-cpu"="sifive-u74" }
+// CHECK: attributes #8 = { {{.*}}"target-cpu"="sifive-x280" "target-features"="+64bit,+a,+m,+save-restore,+zifencei,-relax,-zbb,-zfa" "tune-cpu"="sifive-u74" }
diff --git a/clang/test/CodeGen/RISCV/riscv-func-attr-target.c b/clang/test/CodeGen/RISCV/riscv-func-attr-target.c
index 506acaba687417..63d8205866d7b4 100644
--- a/clang/test/CodeGen/RISCV/riscv-func-attr-target.c
+++ b/clang/test/CodeGen/RISCV/riscv-func-attr-target.c
@@ -37,11 +37,11 @@ __attribute__((target("cpu=sifive-u54"))) void testAttrCpuOnly() {}
 
 //.
 // CHECK: attributes #0 = { {{.*}}"target-features"="+64bit,+a,+m,+save-restore,+zifencei,-relax,-zbb,-zfa" }
-// CHECK: attributes #1 = { {{.*}}"target-cpu"="rocket-rv64" "target-features"="+64bit,+a,+d,+f,+m,+save-restore,+v,+zicsr,+zifencei,+zve32f,+zve32x,+zve64d,+zve64f,+zve64x,+zvl128b,+zvl32b,+zvl64b,-relax,-zbb,-zfa" "tune-cpu"="generic-rv64" }
+// CHECK: attributes #1 = { {{.*}}"target-features"="+64bit,+a,+d,+f,+m,+save-restore,+v,+zicsr,+zifencei,+zve32f,+zve32x,+zve64d,+zve64f,+zve64x,+zvl128b,+zvl32b,+zvl64b,-relax,-zbb,-zfa" "tune-cpu"="generic-rv64" }
 // CHECK: attributes #2 = { {{.*}}"target-features"="+64bit,+a,+m,+save-restore,+zbb,+zifencei,-relax,-zfa" }
 // CHECK: attributes #3 = { {{.*}}"target-features"="+64bit,+a,+d,+experimental-zicond,+f,+m,+save-restore,+v,+zbb,+zicsr,+zifencei,+zve32f,+zve32x,+zve64d,+zve64f,+zve64x,+zvl128b,+zvl32b,+zvl64b,-relax,-zfa" }
-// CHECK: attributes #4 = { {{.*}}"target-features"="+64bit,+a,+c,+d,+f,+m,+save-restore,+zbb,+zicsr,+zifencei,-relax,-zfa" }
-// CHECK: attributes #5 = { {{.*}}"target-features"="+64bit,+m,+save-restore,-relax,-zbb,-zfa" }
-// CHECK: attributes #6 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+a,+m,+save-restore,+zbb,+zifencei,-relax,-zfa" }
-// CHECK: attributes #7 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+m,+save-restore,-relax,-zbb,-zfa" }
+// CHECK: attributes #4 = { {{.*}}"target-cpu"="generic-rv64" "target-features"="+64bit,+a,+c,+d,+f,+m,+save-restore,+zbb,+zicsr,+zifencei,-relax,-zfa" }
+// CHECK: attributes #5 = { {{.*}}"target-cpu"="generic-rv64" "target-features"="+64bit,+m,+save-restore,-relax,-zbb,-zfa" }
+// CHECK: attributes #6 = { {{.*}}"target-features"="+64bit,+a,+m,+save-restore,+zbb,+zifencei,-relax,-zfa" "tune-cpu"="sifive-u54" }
+// CHECK: attributes #7 = { {{.*}}"target-cpu"="generic-rv64" "target-features"="+64bit,+m,+save-restore,-relax,-zbb,-zfa" "tune-cpu"="sifive-u54" }
 // CHECK: attributes #8 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+a,+c,+d,+f,+m,+save-restore,+zicsr,+zifencei,-relax,-zbb,-zfa" }
diff --git a/llvm/test/CodeGen/RISCV/riscv-func-target-feature-mcpu-override.ll b/llvm/test/CodeGen/RISCV/riscv-func-target-feature-mcpu-override.ll
new file mode 100644
index 00000000000000..5057384d57cdf9
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/riscv-func-target-feature-mcpu-override.ll
@@ -0,0 +1,36 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 4
+; RUN: llc -mtriple=riscv64 -mcpu=sifive-x280 -verify-machineinstrs < %s | FileCheck %s
+
+define <2 x i16> @vwaddu_v2i16_without_custom_target_cpu(ptr %x, ptr %y) "target-feature"="+64bit,+a,+m,+c,+f" {
+; CHECK-LABEL: vwaddu_v2i16_without_custom_target_cpu:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    vsetivli zero, 2, e8, mf8, ta, ma
+; CHECK-NEXT:    vle8.v v9, (a0)
+; CHECK-NEXT:    vle8.v v10, (a1)
+; CHECK-NEXT:    vwaddu.vv v8, v9, v10
+; CHECK-NEXT:    ret
+  %a = load <2 x i8>, ptr %x
+  %b = load <2 x i8>, ptr %y
+  %c = zext <2 x i8> %a to <2 x i16>
+  %d = zext <2 x i8> %b to <2 x i16>
+  %e = add <2 x i16> %c, %d
+  ret <2 x i16> %e
+}
+
+define <2 x i16> @vwaddu_v2i16_with_custom_target_cpu(ptr %x, ptr %y) "target-cpu"="generic-rv64" "target-feature"="+64bit,+a,+m,+c,+f" {
+; CHECK-LABEL: vwaddu_v2i16_with_custom_target_cpu:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    lbu a2, 1(a0)
+; CHECK-NEXT:    lbu a0, 0(a0)
+; CHECK-NEXT:    lbu a3, 0(a1)
+; CHECK-NEXT:    lbu a1, 1(a1)
+; CHECK-NEXT:    add a0, a0, a3
+; CHECK-NEXT:    add a1, a2, a1
+; CHECK-NEXT:    ret
+  %a = load <2 x i8>, ptr %x
+  %b = load <2 x i8>, ptr %y
+  %c = zext <2 x i8> %a to <2 x i16>
+  %d = zext <2 x i8> %b to <2 x i16>
+  %e = add <2 x i16> %c, %d
+  ret <2 x i16> %e
+}

@efriedma-quic
Copy link
Collaborator

It doesn't seem like a good idea to make target-independent logic behave in target-specific ways; that's going to confusing for both people hacking on clang, and for users if it's user-visible. Is there some way we can make this logic consistent across targets?

Maybe we can explicitly translate the "arch" feature into enabling/disabling all the relevant features?

@llvmbot llvmbot added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Dec 28, 2023
lukel97 added a commit to lukel97/llvm-project that referenced this pull request Jan 9, 2024
…attribute

This patch reworks RISCVTargetInfo::initFeatureMap to fix the issue described
in llvm#74889 (review)
(and is an alternative to llvm#75804)

When a full arch string is specified, a "full" list of extensions is now passed
after the __RISCV_TargetAttrNeedOverride marker feature, which includes any
negative features that disable ISA extensions.

In initFeatureMap, there are now two code paths:

1. If the arch string was overriden, use the "full" list of override features,
only adding back any non-isa features that were specified.

Using the full list of positive and negative features will mean that the
target-cpu will have no effect on the final arch, e.g.
__attribute__((target("arch=rv64i"))) with -mcpu=sifive-x280 will have the
features for rv64i, not a mix of both.

2. Otherwise, parse and *append* the list of implied features. By appending, we
turn back on any features that might have been disabled by a negative
extension, i.e. this handles the case fixed in llvm#74889.
lukel97 added a commit that referenced this pull request Jan 17, 2024
…attribute (#77426)

This patch reworks RISCVTargetInfo::initFeatureMap to fix the issue
described
in
#74889 (review)
(and is an alternative to #75804)

When a full arch string is specified, a "full" list of extensions is now
passed
after the __RISCV_TargetAttrNeedOverride marker feature, which includes
any
negative features that disable ISA extensions.

In initFeatureMap, there are now two code paths:

1. If the arch string was overriden, use the "full" list of override
features,
only adding back any non-isa features that were specified.

Using the full list of positive and negative features will mean that the
target-cpu will have no effect on the final arch, e.g.
__attribute__((target("arch=rv64i"))) with -mcpu=sifive-x280 will have
the
features for rv64i, not a mix of both.

2. Otherwise, parse and *append* the list of implied features. By
appending, we
turn back on any features that might have been disabled by a negative
extension, i.e. this handles the case fixed in #74889.
// use the baseline cpu.
if (llvm::find(Ret.Features, "__RISCV_TargetAttrNeedOverride") !=
Ret.Features.end())
Ret.CPU =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this here is confusing because it is possible to specify a CPU that gets overridden if there is a full arch string. I think this drops the scheduler model which was not intended?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scheduler model should come from Tune shouldn't it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like RISCVProcessorModel takes a name n which I think corresponds to the mcpu name, and a SchedMachineModel m which I thought was the SchedModel. But I also see that RISCVTuneProcessorModel seems to tie the SchedModel to the Tune. So maybe the answer is that it can come from both?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RISCVProcessorModel defines names that can be used for -mcpu and -mtune. RISCVTuneProcessorModel defines name that can only be used for -mtune. These just associate scheduler models with strings. You need to look at C++ code to see how -mcpu and -mtune are propagated.

This code gets the TuneCPU from the tune-cpu function attribute if present. Otherwise its the same as the CPU. Where CPU either comes from the target-cpu attribute or the TargetCPU in TargetMachine. There is no TuneCPU in TargetMachine so it can only be set differently through the tune-cpu attribute.

const RISCVSubtarget *                                                           
RISCVTargetMachine::getSubtargetImpl(const Function &F) const {                  
  Attribute CPUAttr = F.getFnAttribute("target-cpu");                            
  Attribute TuneAttr = F.getFnAttribute("tune-cpu");                             
  Attribute FSAttr = F.getFnAttribute("target-features");                        
                                                                                 
  std::string CPU =                                                              
      CPUAttr.isValid() ? CPUAttr.getValueAsString().str() : TargetCPU;          
  std::string TuneCPU =                                                          
      TuneAttr.isValid() ? TuneAttr.getValueAsString().str() : CPU;

The lookup function for the scheduler is here

void MCSubtargetInfo::InitMCProcessorInfo(StringRef CPU, StringRef TuneCPU,      
                                          StringRef FS) {                        
  FeatureBits = getFeatures(CPU, TuneCPU, FS, ProcDesc, ProcFeatures);           
  FeatureString = std::string(FS);                                               
                                                                                 
  if (!TuneCPU.empty())                                                          
    CPUSchedModel = &getSchedModelForCPU(TuneCPU);                               
  else                                                                           
    CPUSchedModel = &MCSchedModel::Default;                                      
} 

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sharing this code. I was looking around for it but couldn't quite find it.

@topperc
Copy link
Collaborator

topperc commented Jan 23, 2024

Is this still needed after #77426

@BeMg
Copy link
Contributor Author

BeMg commented Jan 23, 2024

Close due to land #77426

@BeMg BeMg closed this Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:RISC-V clang:codegen clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants