Skip to content

[clang][AVR] Improve compatibility of inline assembly with avr-gcc #136534

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

benshi001
Copy link
Member

Allow the value 64 to be round up to 0 for constraint 'I'.

Allow the value 64 to be round up to 0 for constraint 'I'.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 21, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 21, 2025

@llvm/pr-subscribers-clang

Author: Ben Shi (benshi001)

Changes

Allow the value 64 to be round up to 0 for constraint 'I'.


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

3 Files Affected:

  • (modified) clang/lib/Basic/Targets/AVR.h (+1-1)
  • (modified) clang/test/CodeGen/avr/avr-inline-asm-constraints.c (+2)
  • (modified) clang/test/CodeGen/avr/avr-unsupported-inline-asm-constraints.c (+1)
diff --git a/clang/lib/Basic/Targets/AVR.h b/clang/lib/Basic/Targets/AVR.h
index 2117ab58e6f30..b2f2711c35435 100644
--- a/clang/lib/Basic/Targets/AVR.h
+++ b/clang/lib/Basic/Targets/AVR.h
@@ -124,7 +124,7 @@ class LLVM_LIBRARY_VISIBILITY AVRTargetInfo : public TargetInfo {
       Info.setAllowsRegister();
       return true;
     case 'I': // 6-bit positive integer constant
-      Info.setRequiresImmediate(0, 63);
+      Info.setRequiresImmediate(0, 64);
       return true;
     case 'J': // 6-bit negative integer constant
       Info.setRequiresImmediate(-63, 0);
diff --git a/clang/test/CodeGen/avr/avr-inline-asm-constraints.c b/clang/test/CodeGen/avr/avr-inline-asm-constraints.c
index 3a956de8db48f..c8d83b4848312 100644
--- a/clang/test/CodeGen/avr/avr-inline-asm-constraints.c
+++ b/clang/test/CodeGen/avr/avr-inline-asm-constraints.c
@@ -71,6 +71,8 @@ void z() {
 void I() {
   // CHECK: call addrspace(0) void asm sideeffect "subi r30, $0", "I"(i16 50)
   asm("subi r30, %0" :: "I"(50));
+  // CHECK: call addrspace(0) void asm sideeffect "subi r30, $0", "I"(i16 64)
+  asm("subi r30, %0" :: "I"(64));
 }
 
 void J() {
diff --git a/clang/test/CodeGen/avr/avr-unsupported-inline-asm-constraints.c b/clang/test/CodeGen/avr/avr-unsupported-inline-asm-constraints.c
index 29f0b69285fa8..52b8d1cb044ca 100644
--- a/clang/test/CodeGen/avr/avr-unsupported-inline-asm-constraints.c
+++ b/clang/test/CodeGen/avr/avr-unsupported-inline-asm-constraints.c
@@ -6,4 +6,5 @@ int foo(void) {
   __asm__ volatile("foo %0, 1" : : "fo" (val)); // expected-error {{invalid input constraint 'fo' in asm}}
   __asm__ volatile("foo %0, 1" : : "Nd" (val)); // expected-error {{invalid input constraint 'Nd' in asm}}
   __asm__ volatile("subi r30, %0" : : "G" (1)); // expected-error {{value '1' out of range for constraint 'G'}}
+  __asm__ volatile("out %0, r20" : : "I" (65)); // expected-error {{value '65' out of range for constraint 'I'}}
 }

@s-barannikov
Copy link
Contributor

gcc doesn't seem to allow it?
https://godbolt.org/z/4zh8TTPac

@benshi001
Copy link
Member Author

benshi001 commented Apr 21, 2025

gcc doesn't seem to allow it? https://godbolt.org/z/4zh8TTPac

avr-gcc allow value 64 for constraint 'I' in very special case, such as #51513.

And my solution is loosen that check in the frontend, but let the AVR backend to deny the illegal constraint value 64.

@s-barannikov
Copy link
Contributor

gcc doesn't seem to allow it? https://godbolt.org/z/4zh8TTPac

avr-gcc allow value 64 for constraint 'I' in very special case, such as #51513.

I guess the test needs to be updated to be representative?

And my solution is loosen that check in the frontend, but let the AVR backend to deny the illegal constraint value 64.

This deserves a comment in code so that other people don't get confused about this and don't try to change it back to 63.

@benshi001 benshi001 requested a review from s-barannikov April 21, 2025 07:33
@benshi001
Copy link
Member Author

gcc doesn't seem to allow it? https://godbolt.org/z/4zh8TTPac

avr-gcc allow value 64 for constraint 'I' in very special case, such as #51513.

I guess the test needs to be updated to be representative?

And my solution is loosen that check in the frontend, but let the AVR backend to deny the illegal constraint value 64.

This deserves a comment in code so that other people don't get confused about this and don't try to change it back to 63.

Thanks.

  1. Comment lines are supplemented.
  2. A reject test in the backend is added.

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.

Thanks, LGTM

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

5 participants