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

[Clang][AArch64] Fix typo with colon-separated syntax for system registers #105608

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

v01dXYZ
Copy link
Contributor

@v01dXYZ v01dXYZ commented Aug 22, 2024

The range for Op0 was set to 1 instead of 3.

The description of e493f17 visually explains the encoding of implementation-defined system registers.

class SysReg<string name, bits<2> op0, bits<3> op1, bits<4> crn, bits<4> crm,
bits<3> op2> : SearchableTable {
let SearchableFields = ["Name", "Encoding"];
let EnumValueField = "Encoding";
string Name = name;
string AltName = name;
bits<16> Encoding;
let Encoding{15-14} = op0;
let Encoding{13-11} = op1;
let Encoding{10-7} = crn;
let Encoding{6-3} = crm;
let Encoding{2-0} = op2;
bit Readable = ?;
bit Writeable = ?;
code Requires = [{ {} }];
}

Gobolt: https://godbolt.org/z/WK9PqPvGE

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:ARM backend:AArch64 clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Aug 22, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Aug 22, 2024

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-backend-arm

Author: None (v01dXYZ)

Changes

The range for Op0 was set to 1 instead of 3.

The description of e493f17 visually explains the encoding of implementation-defined system registers.

Gobolt: https://godbolt.org/z/WK9PqPvGE (comment out the last one to convince yourself they represent the same system register).


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

2 Files Affected:

  • (modified) clang/lib/Sema/SemaARM.cpp (+4-4)
  • (modified) clang/test/Sema/aarch64-special-register.c (+7)
diff --git a/clang/lib/Sema/SemaARM.cpp b/clang/lib/Sema/SemaARM.cpp
index d8dd4fe16e3af0..cb53c61aa857d7 100644
--- a/clang/lib/Sema/SemaARM.cpp
+++ b/clang/lib/Sema/SemaARM.cpp
@@ -248,16 +248,16 @@ bool SemaARM::BuiltinARMSpecialReg(unsigned BuiltinID, CallExpr *TheCall,
       }
     }
 
-    SmallVector<int, 5> Ranges;
+    SmallVector<int, 5> FieldBitWidths;
     if (FiveFields)
-      Ranges.append({IsAArch64Builtin ? 1 : 15, 7, 15, 15, 7});
+      FieldBitWidths.append({IsAArch64Builtin ? 2 : 4, 3, 4, 4, 3});
     else
-      Ranges.append({15, 7, 15});
+      FieldBitWidths.append({4, 3, 4});
 
     for (unsigned i = 0; i < Fields.size(); ++i) {
       int IntField;
       ValidString &= !Fields[i].getAsInteger(10, IntField);
-      ValidString &= (IntField >= 0 && IntField <= Ranges[i]);
+      ValidString &= (IntField >= 0 && IntField < (1 << FieldBitWidths[i]));
     }
 
     if (!ValidString)
diff --git a/clang/test/Sema/aarch64-special-register.c b/clang/test/Sema/aarch64-special-register.c
index 4d2cfd8b37c847..53a2c32b84777c 100644
--- a/clang/test/Sema/aarch64-special-register.c
+++ b/clang/test/Sema/aarch64-special-register.c
@@ -116,6 +116,13 @@ unsigned long rsr64_6(void) {
   return __builtin_arm_rsr64("0:1:16:16:2"); //expected-error {{invalid special register for builtin}}
 }
 
+void rsr64_7(unsigned long *r) {
+  // the following three instructions should produce the same assembly
+  r[0] = __builtin_arm_rsr64("ICC_CTLR_EL3");
+  r[1] = __builtin_arm_rsr64("s3_6_c12_c12_4");
+  r[2] = __builtin_arm_rsr64("3:6:12:12:4");
+}
+
 __uint128_t rsr128_3(void) {
   return __builtin_arm_rsr128("0:1:2"); //expected-error {{invalid special register for builtin}}
 }

…sters

The range for Op0 was set to 1 instead of 3. Since the ranges are all
power of 2 minus 1, this commit reformulates them with the bitwidth of
each part of the register to highlight the association with the
encoding.

cf one commit description explaining the encoding of an
implementation-defined system register:

e493f17
@v01dXYZ v01dXYZ force-pushed the clang-aarch64-fix-typo-colon-separated-syntax-system-register branch from 4abe7a1 to faef073 Compare August 22, 2024 03:25
@lenary
Copy link
Member

lenary commented Aug 22, 2024

The existing implementation lines up with the ACLE, which specifies this colon-separated syntax: https://github.com/ARM-software/acle/blob/main/main/acle.md#aarch64-system-register - please send an update to that document before changing this behaviour.

@v01dXYZ
Copy link
Contributor Author

v01dXYZ commented Aug 22, 2024

@lenary PR to update ACLE doc: ARM-software/acle#342

Copy link
Contributor

@nasherm nasherm left a comment

Choose a reason for hiding this comment

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

LGTM. Relevant documentation has been updated as well. Approving unless there's other comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:AArch64 backend:ARM 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.

4 participants