[PowerPC] Enable indiviual crbits tracking at -O2 #133617

mustartt · 2025-03-30T04:30:07Z

Fixes the bug where if the frontend passes target-features, the backend computeFSAdditions will not be able to override the value set by the frontend.

This also moves the target-features attribute for crbits to the backend only, so Clang is only responsible for setting -mcrbits and -mno-crbits.

https://reviews.llvm.org/D124060
This patch turns on support for CR bit accesses for Power8 and above. The reason why CR bits are turned
on as the default for Power8 and above is that because later architectures make use of builtins and
instructions that require CR bit accesses (such as the use of setbc in the vector string isolate predicate
and bcd builtins on Power10).

llvmbot · 2025-03-30T04:30:39Z

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

@llvm/pr-subscribers-clang

Author: Henry Jiang (mustartt)

Changes

https://reviews.llvm.org/D124060

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

5 Files Affected:

(modified) clang/lib/Basic/Targets/PPC.cpp (-5)
(modified) llvm/lib/Target/PowerPC/PPC.td (+43-39)
(modified) llvm/lib/Target/PowerPC/PPCSubtarget.cpp (+9)
(modified) llvm/lib/Target/PowerPC/PPCSubtarget.h (+1)
(modified) llvm/lib/Target/PowerPC/PPCTargetMachine.cpp (-7)

diff --git a/clang/lib/Basic/Targets/PPC.cpp b/clang/lib/Basic/Targets/PPC.cpp index 425ad68bb9098..61d567892b498 100644 --- a/clang/lib/Basic/Targets/PPC.cpp +++ b/clang/lib/Basic/Targets/PPC.cpp @@ -559,11 +559,6 @@ bool PPCTargetInfo::initFeatureMap( .Case("pwr9", true) .Case("pwr8", true) .Default(false); - Features["crbits"] = llvm::StringSwitch<bool>(CPU) - .Case("ppc64le", true) - .Case("pwr9", true) - .Case("pwr8", true) - .Default(false); Features["vsx"] = llvm::StringSwitch<bool>(CPU) .Case("ppc64le", true) .Case("pwr9", true) diff --git a/llvm/lib/Target/PowerPC/PPC.td b/llvm/lib/Target/PowerPC/PPC.td index 39da428461393..9f0f271b619c7 100644 --- a/llvm/lib/Target/PowerPC/PPC.td +++ b/llvm/lib/Target/PowerPC/PPC.td @@ -74,7 +74,7 @@ def Feature64BitRegs : SubtargetFeature<"64bitregs","Use64BitRegs", "true", // Specify if we should store and manipulate i1 values in the individual // condition register bits. -def FeatureCRBits : SubtargetFeature<"crbits", "UseCRBits", "true", +def FeatureCRBits : SubtargetFeature<"crbits", "HasCRBits", "true", "Use condition-register bits individually">; def FeatureFPU : SubtargetFeature<"fpu","HasFPU","true", "Enable classic FPU instructions", @@ -390,6 +390,7 @@ def ProcessorFeatures { FeatureFPCVT, FeatureISEL, FeaturePOPCNTD, + FeatureCRBits, FeatureCMPB, FeatureLDBRX, Feature64Bit, @@ -577,79 +578,82 @@ include "GISel/PPCRegisterBanks.td" // def : Processor<"generic", G3Itineraries, [Directive32, FeatureHardFloat, - FeatureMFTB]>; + FeatureMFTB, FeatureCRBits]>; def : ProcessorModel<"440", PPC440Model, [Directive440, FeatureISEL, FeatureFRES, FeatureFRSQRTE, FeatureICBT, FeatureBookE, - FeatureMSYNC, FeatureMFTB]>; + FeatureMSYNC, FeatureMFTB, + FeatureCRBits]>; def : ProcessorModel<"450", PPC440Model, [Directive440, FeatureISEL, FeatureFRES, FeatureFRSQRTE, FeatureICBT, FeatureBookE, - FeatureMSYNC, FeatureMFTB]>; -def : Processor<"601", G3Itineraries, [Directive601, FeatureFPU]>; + FeatureMSYNC, FeatureMFTB, + FeatureCRBits]>; +def : Processor<"601", G3Itineraries, [Directive601, FeatureFPU, + FeatureCRBits]>; def : Processor<"602", G3Itineraries, [Directive602, FeatureFPU, - FeatureMFTB]>; -def : Processor<"603", G3Itineraries, [Directive603, - FeatureFRES, FeatureFRSQRTE, - FeatureMFTB]>; -def : Processor<"603e", G3Itineraries, [Directive603, - FeatureFRES, FeatureFRSQRTE, - FeatureMFTB]>; + FeatureMFTB, FeatureCRBits]>; +def : Processor<"603", G3Itineraries, [Directive603, FeatureFRES, + FeatureFRSQRTE, FeatureMFTB, + FeatureCRBits]>; +def : Processor<"603e", G3Itineraries, [Directive603, FeatureFRES, + FeatureFRSQRTE, FeatureMFTB, + FeatureCRBits]>; def : Processor<"603ev", G3Itineraries, [Directive603, FeatureFRES, FeatureFRSQRTE, - FeatureMFTB]>; + FeatureMFTB, FeatureCRBits]>; def : Processor<"604", G3Itineraries, [Directive604, FeatureFRES, FeatureFRSQRTE, - FeatureMFTB]>; + FeatureMFTB, FeatureCRBits]>; def : Processor<"604e", G3Itineraries, [Directive604, FeatureFRES, FeatureFRSQRTE, - FeatureMFTB]>; + FeatureMFTB, FeatureCRBits]>; def : Processor<"620", G3Itineraries, [Directive620, FeatureFRES, FeatureFRSQRTE, - FeatureMFTB]>; + FeatureMFTB, FeatureCRBits]>; def : Processor<"750", G4Itineraries, [Directive750, FeatureFRES, FeatureFRSQRTE, - FeatureMFTB]>; + FeatureMFTB, FeatureCRBits]>; def : Processor<"g3", G3Itineraries, [Directive750, FeatureFRES, FeatureFRSQRTE, - FeatureMFTB]>; + FeatureMFTB, FeatureCRBits]>; def : Processor<"7400", G4Itineraries, [Directive7400, FeatureAltivec, FeatureFRES, FeatureFRSQRTE, - FeatureMFTB]>; + FeatureMFTB, FeatureCRBits]>; def : Processor<"g4", G4Itineraries, [Directive7400, FeatureAltivec, FeatureFRES, FeatureFRSQRTE, - FeatureMFTB]>; + FeatureMFTB, FeatureCRBits]>; def : Processor<"7450", G4PlusItineraries, [Directive7400, FeatureAltivec, FeatureFRES, FeatureFRSQRTE, - FeatureMFTB]>; + FeatureMFTB, FeatureCRBits]>; def : Processor<"g4+", G4PlusItineraries, [Directive7400, FeatureAltivec, FeatureFRES, FeatureFRSQRTE, - FeatureMFTB]>; + FeatureMFTB, FeatureCRBits]>; def : ProcessorModel<"970", G5Model, [Directive970, FeatureAltivec, FeatureMFOCRF, FeatureFSqrt, FeatureFRES, FeatureFRSQRTE, FeatureSTFIWX, Feature64Bit /*, Feature64BitRegs */, - FeatureMFTB]>; + FeatureMFTB, FeatureCRBits]>; def : ProcessorModel<"g5", G5Model, [Directive970, FeatureAltivec, FeatureMFOCRF, FeatureFSqrt, FeatureSTFIWX, FeatureFRES, FeatureFRSQRTE, Feature64Bit /*, Feature64BitRegs */, - FeatureMFTB, DeprecatedDST]>; + FeatureMFTB, DeprecatedDST, FeatureCRBits]>; def : ProcessorModel<"e500", PPCE500Model, - [DirectiveE500, - FeatureICBT, FeatureBookE, - FeatureISEL, FeatureMFTB, FeatureMSYNC, FeatureSPE]>; + [DirectiveE500, FeatureICBT, FeatureBookE, + FeatureISEL, FeatureMFTB, FeatureMSYNC, + FeatureSPE, FeatureCRBits]>; def : ProcessorModel<"e500mc", PPCE500mcModel, [DirectiveE500mc, FeatureSTFIWX, FeatureICBT, FeatureBookE, - FeatureISEL, FeatureMFTB]>; + FeatureISEL, FeatureMFTB, FeatureCRBits]>; def : ProcessorModel<"e5500", PPCE5500Model, [DirectiveE5500, FeatureMFOCRF, Feature64Bit, FeatureSTFIWX, FeatureICBT, FeatureBookE, - FeatureISEL, FeatureMFTB]>; + FeatureISEL, FeatureMFTB, FeatureCRBits]>; def : ProcessorModel<"a2", PPCA2Model, [DirectiveA2, FeatureICBT, FeatureBookE, FeatureMFOCRF, FeatureFCPSGN, FeatureFSqrt, FeatureFRE, FeatureFRES, @@ -658,41 +662,41 @@ def : ProcessorModel<"a2", PPCA2Model, FeatureFPRND, FeatureFPCVT, FeatureISEL, FeatureSlowPOPCNTD, FeatureCMPB, FeatureLDBRX, Feature64Bit /*, Feature64BitRegs */, FeatureMFTB, - FeatureISA2_06]>; + FeatureISA2_06, FeatureCRBits]>; def : ProcessorModel<"pwr3", G5Model, [DirectivePwr3, FeatureAltivec, FeatureFRES, FeatureFRSQRTE, FeatureMFOCRF, - FeatureSTFIWX, Feature64Bit]>; + FeatureSTFIWX, Feature64Bit, FeatureCRBits]>; def : ProcessorModel<"pwr4", G5Model, [DirectivePwr4, FeatureAltivec, FeatureMFOCRF, FeatureFSqrt, FeatureFRES, FeatureFRSQRTE, - FeatureSTFIWX, Feature64Bit, FeatureMFTB]>; + FeatureSTFIWX, Feature64Bit, FeatureMFTB, FeatureCRBits]>; def : ProcessorModel<"pwr5", G5Model, [DirectivePwr5, FeatureAltivec, FeatureMFOCRF, FeatureFSqrt, FeatureFRE, FeatureFRES, FeatureFRSQRTE, FeatureFRSQRTES, FeatureSTFIWX, Feature64Bit, - FeatureMFTB, DeprecatedDST]>; + FeatureMFTB, DeprecatedDST, FeatureCRBits]>; def : ProcessorModel<"pwr5x", G5Model, [DirectivePwr5x, FeatureAltivec, FeatureMFOCRF, FeatureFSqrt, FeatureFRE, FeatureFRES, FeatureFRSQRTE, FeatureFRSQRTES, FeatureSTFIWX, FeatureFPRND, Feature64Bit, - FeatureMFTB, DeprecatedDST]>; + FeatureMFTB, DeprecatedDST, FeatureCRBits]>; def : ProcessorModel<"pwr6", G5Model, [DirectivePwr6, FeatureAltivec, FeatureMFOCRF, FeatureFCPSGN, FeatureFSqrt, FeatureFRE, FeatureFRES, FeatureFRSQRTE, FeatureFRSQRTES, FeatureRecipPrec, FeatureSTFIWX, FeatureLFIWAX, FeatureCMPB, FeatureFPRND, Feature64Bit /*, Feature64BitRegs */, - FeatureMFTB, DeprecatedDST]>; + FeatureMFTB, DeprecatedDST, FeatureCRBits]>; def : ProcessorModel<"pwr6x", G5Model, [DirectivePwr5x, FeatureAltivec, FeatureMFOCRF, FeatureFCPSGN, FeatureFSqrt, FeatureFRE, FeatureFRES, FeatureFRSQRTE, FeatureFRSQRTES, FeatureRecipPrec, FeatureSTFIWX, FeatureLFIWAX, FeatureCMPB, FeatureFPRND, Feature64Bit, - FeatureMFTB, DeprecatedDST]>; + FeatureMFTB, DeprecatedDST, FeatureCRBits]>; def : ProcessorModel<"pwr7", P7Model, ProcessorFeatures.P7Features>; def : ProcessorModel<"pwr8", P8Model, ProcessorFeatures.P8Features>; def : ProcessorModel<"pwr9", P9Model, ProcessorFeatures.P9Features>; @@ -702,15 +706,15 @@ def : ProcessorModel<"pwr11", P10Model, ProcessorFeatures.P11Features>; def : ProcessorModel<"future", NoSchedModel, ProcessorFeatures.FutureFeatures>; def : Processor<"ppc", G3Itineraries, [Directive32, FeatureHardFloat, - FeatureMFTB]>; + FeatureMFTB, FeatureCRBits]>; def : Processor<"ppc32", G3Itineraries, [Directive32, FeatureHardFloat, - FeatureMFTB]>; + FeatureMFTB, FeatureCRBits]>; def : ProcessorModel<"ppc64", G5Model, [Directive64, FeatureAltivec, FeatureMFOCRF, FeatureFSqrt, FeatureFRES, FeatureFRSQRTE, FeatureSTFIWX, Feature64Bit /*, Feature64BitRegs */, - FeatureMFTB]>; + FeatureMFTB, FeatureCRBits]>; def : ProcessorModel<"ppc64le", P8Model, ProcessorFeatures.P8Features>; //===----------------------------------------------------------------------===// diff --git a/llvm/lib/Target/PowerPC/PPCSubtarget.cpp b/llvm/lib/Target/PowerPC/PPCSubtarget.cpp index 75a0272af7c31..746358d273dff 100644 --- a/llvm/lib/Target/PowerPC/PPCSubtarget.cpp +++ b/llvm/lib/Target/PowerPC/PPCSubtarget.cpp @@ -26,6 +26,7 @@ #include "llvm/IR/GlobalValue.h" #include "llvm/IR/GlobalVariable.h" #include "llvm/MC/TargetRegistry.h" +#include "llvm/Support/CodeGen.h" #include "llvm/Support/CommandLine.h" #include "llvm/Target/TargetMachine.h" #include "llvm/TargetParser/PPCTargetParser.h" @@ -149,6 +150,14 @@ void PPCSubtarget::initSubtargetFeatures(StringRef CPU, StringRef TuneCPU, false); } +bool PPCSubtarget::useCRBits() const { + if (!hasCRBits()) + return false; + if (CPUDirective >= PPC::DIR_PWR8) + return true; + return TM.getOptLevel() >= CodeGenOptLevel::Default; +} + bool PPCSubtarget::enableMachineScheduler() const { return true; } bool PPCSubtarget::enableMachinePipeliner() const { diff --git a/llvm/lib/Target/PowerPC/PPCSubtarget.h b/llvm/lib/Target/PowerPC/PPCSubtarget.h index 9a97d1aa4dab0..fcb17d71e8e7c 100644 --- a/llvm/lib/Target/PowerPC/PPCSubtarget.h +++ b/llvm/lib/Target/PowerPC/PPCSubtarget.h @@ -209,6 +209,7 @@ class PPCSubtarget : public PPCGenSubtargetInfo { } POPCNTDKind hasPOPCNTD() const { return HasPOPCNTD; } + bool useCRBits() const;  const Triple &getTargetTriple() const { return TargetTriple; } diff --git a/llvm/lib/Target/PowerPC/PPCTargetMachine.cpp b/llvm/lib/Target/PowerPC/PPCTargetMachine.cpp index 5ee13a92cf993..fa611f831a09a 100644 --- a/llvm/lib/Target/PowerPC/PPCTargetMachine.cpp +++ b/llvm/lib/Target/PowerPC/PPCTargetMachine.cpp @@ -210,13 +210,6 @@ static std::string computeFSAdditions(StringRef FS, CodeGenOptLevel OL, FullFS = "+64bit"; } - if (OL >= CodeGenOptLevel::Default) { - if (!FullFS.empty()) - FullFS = "+crbits," + FullFS; - else - FullFS = "+crbits"; - } - if (OL != CodeGenOptLevel::None) { if (!FullFS.empty()) FullFS = "+invariant-function-descriptors," + FullFS;

github-actions · 2025-03-30T04:32:34Z

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

w2yehia · 2025-04-01T14:59:59Z

clang/test/Driver/ppc-crbits.cpp

-// RUN: -emit-llvm -S %s -o - | FileCheck %s --check-prefix=HAS-NOCRBITS
-
-
 // HAS-CRBITS: main(


this is dead code if the above is removed.

w2yehia · 2025-04-01T15:00:39Z

clang/test/Driver/ppc-crbits.cpp


-// RUN: %clang -target powerpc64le-unknown-linux-gnu -mcpu=pwr10 -emit-llvm \
-// RUN: -S %s -o - | FileCheck %s --check-prefix=HAS-CRBITS
-// RUN: %clang -target powerpc64le-unknown-linux-gnu -mcpu=pwr10 -mcrbits \


is the -mcrbits option no longer supported? if it is supported, then should we keep these tests?

The -mcrbits and -mno-crbits flags are still supported. But specifying -mcpu= in conjunction with -mcrbits does not do anything so the tests are that tests for these are removed.

Now on second thought, I will clean this entire test to keep only 2 branches for th e on and off flags.

// RUN: %clang -target powerpc-ibm-aix %s -### \ // RUN: -mcrbits -o %t.o 2>&1 | FileCheck -check-prefix=CHECK-CRBITS %s // RUN: %clang -target powerpc-ibm-aix %s -### \ // RUN: -mno-crbits -o %t.o 2>&1 | FileCheck -check-prefix=CHECK-NOCRBITS %s

Removing -mcpu means -mcrbits/-mno-crbits are only tested with the default cpu.

I think I agree with Maryam here. Might be good to see if the CR bits feature is on depending on what CPU level is specified.

Thanks for the suggestions, I will update the tests accordingly.

maryammo

Can we have a test where it shows with this PR, the backend computeFSAdditions can override the CRBits passed by frontend?

maryammo · 2025-04-02T15:37:46Z

llvm/test/CodeGen/PowerPC/crbit-asm.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 3
 ; RUN: llc -verify-machineinstrs -mcpu=pwr7 < %s | FileCheck %s
-; RUN: llc -verify-machineinstrs -O1 -mcpu=pwr7 < %s | FileCheck %s
+; RUN: llc -verify-machineinstrs -O2 -mcpu=pwr7 < %s | FileCheck %s


Why changing the opt level here? seems it does not affect the code-gen.

I assume maybe it is to test that we generate the CR bit instructions for -O2+. Perhaps we can either introduce a new test for this, or add another -O2 line here to check that the -O2 codegen evaluates to the same existing codegen.

I agree with adding another O2 line here to clarify that the CR bit instructions are being generated at O2+ with this patch, and still keep the O1 line.

amy-kwan

Btw, thank you for fixing the bug in the original PR! Appreciate it. :-)

amy-kwan · 2025-04-02T19:13:57Z

llvm/lib/Target/PowerPC/PPC.td


 def : Processor<"generic", G3Itineraries, [Directive32, FeatureHardFloat,
- FeatureMFTB]>;
+ FeatureMFTB, FeatureCRBits]>;


This next few lines below basically mean the following processors contain CR bits (but won't be on, because we turn them on for P8+), right? Did we use a particular way to verify for certain that these processors have CR bit support?

mustartt · 2025-08-20T15:46:13Z

@diggerlin Is this still necessary?
I believe the CRBits inconsistency was sorted out already

llvmbot added clang Clang issues not falling into any other category backend:PowerPC clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Mar 30, 2025

Enable indiviual crbits tracking at O2

8a71c3b

mustartt force-pushed the crbits branch from d10bfa5 to 8a71c3b Compare March 31, 2025 20:39

update test to test for flags only

ef8b93e

llvmbot added the clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' label Mar 31, 2025

mustartt requested review from amy-kwan, diggerlin and nemanjai March 31, 2025 20:49

w2yehia reviewed Apr 1, 2025

View reviewed changes

maryammo reviewed Apr 2, 2025

View reviewed changes

amy-kwan reviewed Apr 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[PowerPC] Enable indiviual crbits tracking at -O2 #133617

[PowerPC] Enable indiviual crbits tracking at -O2 #133617

Uh oh!

mustartt commented Mar 30, 2025 •

edited

Loading

llvmbot commented Mar 30, 2025 •

edited

Loading

github-actions bot commented Mar 30, 2025 •

edited

Loading

w2yehia Apr 1, 2025

w2yehia Apr 1, 2025

mustartt Apr 1, 2025

maryammo Apr 1, 2025

amy-kwan Apr 2, 2025

mustartt Apr 2, 2025

maryammo left a comment

maryammo Apr 2, 2025 •

edited

Loading

amy-kwan Apr 2, 2025

maryammo Apr 2, 2025

amy-kwan left a comment

amy-kwan Apr 2, 2025

mustartt commented Aug 20, 2025

Labels

5 participants

		// RUN: -emit-llvm -S %s -o - \| FileCheck %s --check-prefix=HAS-NOCRBITS


		// HAS-CRBITS: main(

[PowerPC] Enable indiviual crbits tracking at -O2 #133617

Are you sure you want to change the base?

[PowerPC] Enable indiviual crbits tracking at -O2 #133617

Uh oh!

Conversation

mustartt commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

llvmbot commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

github-actions bot commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryammo left a comment

Choose a reason for hiding this comment

maryammo Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-kwan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustartt commented Aug 20, 2025

Labels

5 participants

mustartt commented Mar 30, 2025 •

edited

Loading

llvmbot commented Mar 30, 2025 •

edited

Loading

github-actions bot commented Mar 30, 2025 •

edited

Loading

maryammo Apr 2, 2025 •

edited

Loading