Skip to content

[ARM] Change the type of CC and VCC code in splitMnemonic. #83413

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

Merged
merged 2 commits into from
Mar 1, 2024

Conversation

AlfieRichardsArm
Copy link
Contributor

This changes the type of PredicationCode and VPTPredicationCode from unsigned to ARMCC::CondCodes and ARMVCC::VPTCodes resp' for clarity and correctness.

@llvmbot
Copy link
Member

llvmbot commented Feb 29, 2024

@llvm/pr-subscribers-mc

@llvm/pr-subscribers-backend-arm

Author: Alfie Richards (AlfieRichardsArm)

Changes

This changes the type of PredicationCode and VPTPredicationCode from unsigned to ARMCC::CondCodes and ARMVCC::VPTCodes resp' for clarity and correctness.


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

1 Files Affected:

  • (modified) llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp (+9-9)
diff --git a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
index 37bfb76a494dee..dd6c637a6fa2a0 100644
--- a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
+++ b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
@@ -506,9 +506,10 @@ class ARMAsmParser : public MCTargetAsmParser {
 
   bool isMnemonicVPTPredicable(StringRef Mnemonic, StringRef ExtraToken);
   StringRef splitMnemonic(StringRef Mnemonic, StringRef ExtraToken,
-                          unsigned &PredicationCode,
-                          unsigned &VPTPredicationCode, bool &CarrySetting,
-                          unsigned &ProcessorIMod, StringRef &ITMask);
+                          ARMCC::CondCodes &PredicationCode,
+                          ARMVCC::VPTCodes &VPTPredicationCode,
+                          bool &CarrySetting, unsigned &ProcessorIMod,
+                          StringRef &ITMask);
   void getMnemonicAcceptInfo(StringRef Mnemonic, StringRef ExtraToken,
                              StringRef FullInst, bool &CanAcceptCarrySet,
                              bool &CanAcceptPredicationCode,
@@ -6283,10 +6284,9 @@ bool ARMAsmParser::parsePrefix(ARMMCExpr::VariantKind &RefKind) {
 //
 // FIXME: Would be nice to autogen this.
 // FIXME: This is a bit of a maze of special cases.
-StringRef ARMAsmParser::splitMnemonic(StringRef Mnemonic,
-                                      StringRef ExtraToken,
-                                      unsigned &PredicationCode,
-                                      unsigned &VPTPredicationCode,
+StringRef ARMAsmParser::splitMnemonic(StringRef Mnemonic, StringRef ExtraToken,
+                                      ARMCC::CondCodes &PredicationCode,
+                                      ARMVCC::VPTCodes &VPTPredicationCode,
                                       bool &CarrySetting,
                                       unsigned &ProcessorIMod,
                                       StringRef &ITMask) {
@@ -6340,7 +6340,7 @@ StringRef ARMAsmParser::splitMnemonic(StringRef Mnemonic,
     unsigned CC = ARMCondCodeFromString(Mnemonic.substr(Mnemonic.size()-2));
     if (CC != ~0U) {
       Mnemonic = Mnemonic.slice(0, Mnemonic.size() - 2);
-      PredicationCode = CC;
+      PredicationCode = (ARMCC::CondCodes)CC;
     }
   }
 
@@ -6387,7 +6387,7 @@ StringRef ARMAsmParser::splitMnemonic(StringRef Mnemonic,
     unsigned CC = ARMVectorCondCodeFromString(Mnemonic.substr(Mnemonic.size()-1));
     if (CC != ~0U) {
       Mnemonic = Mnemonic.slice(0, Mnemonic.size()-1);
-      VPTPredicationCode = CC;
+      VPTPredicationCode = (ARMVCC::VPTCodes)CC;
     }
     return Mnemonic;
   }

This changes the type from `unsigned` to `ARMCC::CondCodes` for clarity and
correctness.
@AlfieRichardsArm
Copy link
Contributor Author

Note the failing test is not relevant, seems to be a timeout or similar

@AlfieRichardsArm
Copy link
Contributor Author

I will wait a couple days for external code review

@AlfieRichardsArm
Copy link
Contributor Author

@s-barannikov I've added you as a reviewer as this commit is a prerequisite for some optional operands work that I will submit in coming days and I see you have worked on the same area.

Copy link
Contributor

@s-barannikov s-barannikov left a comment

Choose a reason for hiding this comment

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

Straightforward change, LGTM modulo style nit

@@ -6340,7 +6340,7 @@ StringRef ARMAsmParser::splitMnemonic(StringRef Mnemonic,
unsigned CC = ARMCondCodeFromString(Mnemonic.substr(Mnemonic.size()-2));
if (CC != ~0U) {
Mnemonic = Mnemonic.slice(0, Mnemonic.size() - 2);
PredicationCode = CC;
PredicationCode = (ARMCC::CondCodes)CC;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The anchor link really changes the meaning of that link (:
I'll get a quick fix in

Copy link

github-actions bot commented Feb 29, 2024

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

@AlfieRichardsArm
Copy link
Contributor Author

Failing test continues to be a seemingly unrelated failure regarding MLIR

@AlfieRichardsArm AlfieRichardsArm force-pushed the change-cc-and-vcc-type branch from 0bc0eda to a1cd51c Compare March 1, 2024 12:44
@llvmbot llvmbot added the mc Machine (object) code label Mar 1, 2024
@AlfieRichardsArm AlfieRichardsArm force-pushed the change-cc-and-vcc-type branch from a1cd51c to bf7bbb1 Compare March 1, 2024 12:46
@AlfieRichardsArm
Copy link
Contributor Author

After a fixing me pushing to the wrong branch this is back to what it should be.

@AlfieRichardsArm
Copy link
Contributor Author

Test failure is unrelated MLIR test. Merging this now.

@AlfieRichardsArm AlfieRichardsArm merged commit b8e0f3e into llvm:main Mar 1, 2024
@AlfieRichardsArm AlfieRichardsArm deleted the change-cc-and-vcc-type branch March 1, 2024 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:ARM mc Machine (object) code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants