Skip to content

Conversation

@syzaara
Copy link
Contributor

@syzaara syzaara commented Jul 24, 2024

We emit an error when -msoft-float and -maltivec/-mvsx is used together, but when -msoft-float is used on its own, there is still +altivec and +vsx in the IR attributes. This patch disables altivec and vsx and all related sub features when -msoft-float is used.

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:PowerPC clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jul 24, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 24, 2024

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

@llvm/pr-subscribers-backend-powerpc

Author: Zaara Syeda (syzaara)

Changes

We emit an error when -msoft-float and -maltivec/-mvsx is used together, but when -msoft-float is used on its own, there is still +altivec and +vsx in the IR attributes. This patch disables altivec and vsx when -msoft-float is used.


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

2 Files Affected:

  • (modified) clang/lib/Basic/Targets/PPC.cpp (+5)
  • (added) clang/test/Driver/ppc-soft-float.c (+13)
diff --git a/clang/lib/Basic/Targets/PPC.cpp b/clang/lib/Basic/Targets/PPC.cpp
index 9ff54083c923b..2b25cb189279a 100644
--- a/clang/lib/Basic/Targets/PPC.cpp
+++ b/clang/lib/Basic/Targets/PPC.cpp
@@ -690,6 +690,11 @@ bool PPCTargetInfo::initFeatureMap(
     return false;
   }
 
+  if (llvm::is_contained(FeaturesVec, "-hard-float")) {
+    Features["altivec"] = false;
+    Features["vsx"] = false;
+  }
+
   return TargetInfo::initFeatureMap(Features, Diags, CPU, FeaturesVec);
 }
 
diff --git a/clang/test/Driver/ppc-soft-float.c b/clang/test/Driver/ppc-soft-float.c
new file mode 100644
index 0000000000000..7b9205662ffeb
--- /dev/null
+++ b/clang/test/Driver/ppc-soft-float.c
@@ -0,0 +1,13 @@
+// RUN: %clang -target powerpc64-unknown-unknown -mcpu=pwr10 -msoft-float -S -emit-llvm %s -o - | FileCheck %s -check-prefix=CHECKSOFT
+// RUN: %clang -target powerpc64-unknown-unknown -mcpu=pwr10 -S -emit-llvm %s -o - | FileCheck %s -check-prefix=CHECKNOSOFT
+
+int main () {
+  return 0;
+}
+
+// CHECKSOFT-DAG: -hard-float
+// CHECKSOFT-DAG: -vsx
+// CHECKSOFT-DAG: -altivec
+
+// CHECKNOSOFT-DAG: +vsx
+// CHECKNOSOFT-DAG: +altivec

Copy link
Collaborator

@chenzheng1030 chenzheng1030 left a comment

Choose a reason for hiding this comment

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

def FeatureFPU       : SubtargetFeature<"fpu","HasFPU","true",
                                        "Enable classic FPU instructions",
                                        [FeatureHardFloat]>;
def FeatureAltivec   : SubtargetFeature<"altivec","HasAltivec", "true",
                                        "Enable Altivec instructions",
                                        [FeatureFPU]>;

If FeatureHardFloat is false, FeatureFPU should be false and FeatureAltivec should be false too.

Seems we are lacking more handling in PPCTargetInfo::setFeatureEnabled(), for example, with this fix, -msoft-float disables vsx and altivec, but it does not disable direct-move, power8-vector....

@syzaara syzaara requested a review from chenzheng1030 July 30, 2024 16:50
Copy link
Collaborator

@chenzheng1030 chenzheng1030 left a comment

Choose a reason for hiding this comment

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

Compared with llvm/lib/Target/PowerPC/PPC.td, seems still miss below implicit flag:
crypto

def FeatureP8Crypto : SubtargetFeature<"crypto", "HasP8Crypto", "true",
                                       "Enable POWER8 Crypto instructions",
                                       [FeatureP8Altivec]>;

Other implications from hard-float are all covered in this patch.

@syzaara syzaara requested a review from chenzheng1030 July 31, 2024 19:50
Copy link
Collaborator

@chenzheng1030 chenzheng1030 left a comment

Choose a reason for hiding this comment

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

We still miss many implications in PPCTargetInfo::setFeatureEnabled(). This patch fixes the implications for -msoft-float. But implications like -mno-power8-altivec for -crypto is still not handled.

Let's leave them for now and make this patch focus on -msoft-float. There will be a refactor for PPC target for the target features based on PPCTargetParser.cpp. I think that time is a better time to handle all other feature implications.

Copy link
Collaborator

@chenzheng1030 chenzheng1030 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for making the fix.

@syzaara syzaara merged commit 3e71357 into llvm:main Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend:PowerPC clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' 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.

3 participants