[Clang][M68k] Add Clang support for the new M68k_RTD CC

[myhsu]

Summary

This patch adds CC_M68kRTD to model llvm::CallingConv::M68k_RTD if either __attribute__((m68k_rtd)) is presented on a function or -mrtd flag is given.

[0x59616e]

clang/lib/Frontend/CompilerInvocation.cpp

559

nit: Is the expanded one better in terms of readability ?

clang/test/CodeGen/mrtd.c

9

Just curious, why do we have to use such an arcane name instead of a more lucid one , such as m68k_rtdcc.

[0x59616e]

clang/test/CodeGen/mrtd.c

9

I guess this involves more work ?

[myhsu]

clang/test/CodeGen/mrtd.c

9

I guess this involves more work ?

Yes because it requires changes to (LLVM IR's) AsmReader/Writer. But it's not hard and I can do that. The reason I didn't do that was simply because this CC is rare so the arcane name's impact on (our) productivity will be low.

[myhsu]

Introduce m68k_rtdcc the textual LLVM IR designation for M68k_RTD

[myhsu]

Minor updates. NFC.

[0x59616e]

I didn't see any issue here. Let's wait for the approval from other (more senior) reviewers.

[jrtc27]

So GCC gives me:

warning: ‘stdcall’ attribute directive ignored [-Wattributes]

when trying to use __attribute__((stdcall)) on m68k, which matches the fact it's only mentioned in the manage for the x86 option.

Interestingly it seems to ICE during expand when I use -mrtd though...

clang/lib/Basic/Targets/M68k.cpp

258

Uhh

clang/lib/CodeGen/CGCall.cpp

51

Ditto...

clang/test/CodeGen/mrtd.c

4

Ew... this should be using -verify and // expected-warning {{...}}, then the line number is relative to the comment's location. Or, really, it should be in the Sema test...

Plus is it correct to call this stdcall on m68k? The GCC manpage only mentions it in the x86 option description, not the m68k one.

[myhsu]

In D149867#4325040, @jrtc27 wrote:

So GCC gives me:

warning: ‘stdcall’ attribute directive ignored [-Wattributes]

when trying to use __attribute__((stdcall)) on m68k, which matches the fact it's only mentioned in the manage for the x86 option.

In principal, I wanted to reuse as much existing code path as possible to handle -mrtd, primarily because these two calling conventions are basically identical and I didn't want to create too much churn. Reusing both DCC_StdCall and CC_X86StdCall serves that purpose. Making __attribute__((stdcall)) available to m68k is just a side effect that I felt harmless.
I'm fine to create a separate CC_M68kRTDCall or even DCC_RTDCall. What do you think @jrtc27 ?

Interestingly it seems to ICE during expand when I use -mrtd though...

I guess you're referring to GCC? I'm curious which snippet you used.

[myhsu]

ping

[0x59616e]

Though creating our own calling convention is better, I think Min's path is correct at this moment given that m68k is still an experimental target. We can reignite this discussion once we're ready to becoming a offiical target.

@myhsu could you open an issue to cite this problem ?

[jrtc27]

I disagree. Being experimental doesn't mean you should do the wrong thing. Reusing stdcall in the frontend is ugly, pollutes non-m68k code paths (doing your own thing _avoids_ that and makes the experimental backend get out of the way, even) and introduces a bug where you can write garbage code and have it compile rather than be rejected like it should be.

Add your own calling convention like everyone else.

[glaubitz]

In D149867#4386271, @jrtc27 wrote:

I disagree. Being experimental doesn't mean you should do the wrong thing. Reusing stdcall in the frontend is ugly, pollutes non-m68k code paths (doing your own thing _avoids_ that and makes the experimental backend get out of the way, even) and introduces a bug where you can write garbage code and have it compile rather than be rejected like it should be.

Maybe we can get the TLS stuff merged first before dealing with this more complex change?

[jrtc27]

In D149867#4387189, @glaubitz wrote:

In D149867#4386271, @jrtc27 wrote:

I disagree. Being experimental doesn't mean you should do the wrong thing. Reusing stdcall in the frontend is ugly, pollutes non-m68k code paths (doing your own thing _avoids_ that and makes the experimental backend get out of the way, even) and introduces a bug where you can write garbage code and have it compile rather than be rejected like it should be.

Maybe we can get the TLS stuff merged first before dealing with this more complex change?

They're totally independent, so nothing's blocking that being merged. Plus this isn't a complex change to make, just a bunch of copy pasta.

[myhsu]

Use a new CC, CC_M68kRTD, to model llvm::CallingConv::M68k_RTD instead of using X86StdCall

[myhsu]

clang/test/CodeGen/mrtd.c

4

Now this check is moved to test/Sema/m68k-mrtd.c

[aaron.ballman]

This should also come with a release note for the changes.

clang/lib/AST/ASTContext.cpp

11998–11999 ↗

(On Diff #530089)

This looks wrong to me -- if the language option is calling for stdcall, saying "here's m68k_rtd instead" does not match caller expectations.

clang/lib/Frontend/CompilerInvocation.cpp

559

Maybe it's too early in the morning for me to be thinking clearly, but this is wrong for m68k, isn't it? If the default calling convention is stdcall and the architecture is m68k, we want to emit the error, don't we?

I don't see test coverage for the change.

clang/lib/Sema/SemaType.cpp

7960–7961 ↗

(On Diff #530089)

Missing test coverage, but also, MSVC supports m68k RTD? I would assume we'd error on this situation given how we document the attribute as not supporting this.

clang/test/CodeGen/mrtd.c

4

This check should be removed here -- we don't typically test diagnostic behavior from codegen tests unless the diagnostic is only emitted during codegen.

clang/test/Sema/m68k-mrtd.c

4–9 ↗

(On Diff #530089)

A better way to do this is to use -verify=mrtd on the line enabling rtd, and using // rtd-error {{whatever}} on the line being diagnosed. (Same comment applies throughout the file.)

Huh, I was unaware that implicit function declarations are using something other than the default calling convention (which is C, not m68k_rtd). Is this intentional?

45 ↗

(On Diff #530089)

Missing tests for:

• Function without a prototype
• Applying the attribute to a non-function
• Providing arguments to the attribute
• What should happen for C++ and things like member functions?

[myhsu]

• Add a new default calling convention -fdefault-calling-conv=rtdcall to model using m68k_rtdcc globally
• Add documents for rtdcall
• Add more tests on __attribute__((m68k_rtd))

[myhsu]

clang/lib/Frontend/CompilerInvocation.cpp

559

The idea was to use our new RTD CC whenever -mrtd is given to either the driver or the frontend. Due to some historical reasons -mrtd has been taken by i386 to specify stdcall. Specifically, DefaultCallingConvention will be set to DCC_StdCall when -mrtd is present. My original approach was to reuse this workflow and the DCC_StdCall, then translate to either CC_X86StdCall or CC_M68kRTD later in ASTContext.

Since there are many concerns on reusing DCC_StdCall, I will create another DCC enum value instead.

clang/test/Sema/m68k-mrtd.c

4–9 ↗

(On Diff #530089)

Huh, I was unaware that implicit function declarations are using something other than the default calling convention (which is C, not m68k_rtd). Is this intentional?

I'm not sure if I understand you correctly, but this diagnostic is emitted if the CC does not support variadic function call.

45 ↗

(On Diff #530089)

Function without a prototype

I thought the first check was testing function without a prototype.

What should happen for C++ and things like member functions?

I believe we don't have any special handling for C++.

I addressed rest of the bullet items you mentioned, please take a look.

[aaron.ballman]

Sorry for the long delay in review on this, it slipped through the cracks for me.

clang/lib/AST/TypePrinter.cpp

1861 ↗

(On Diff #532423)

clang/test/Sema/m68k-mrtd.c

4–9 ↗

(On Diff #530089)

bar is not declared, in C89 this causes an implicit function declaration to be generated with the signature: extern int ident(); What surprised me is that we seem to be generating something more like this instead: __attribute__((m68k_rtd)) int ident(); despite that not being valid.

45 ↗

(On Diff #530089)

Function without a prototype

I thought the first check was testing function without a prototype.

Nope, I meant something more like this:

__attribute__((m68k_rtd)) void foo(); // Should error
__attribute__((m68k_rtd)) void bar(a, b) int a, b; {} // Should error

What should happen for C++ and things like member functions?

I believe we don't have any special handling for C++.

Test coverage should still exist to ensure the behavior is what you'd expect when adding the calling convention to a member function and a lambda, for example. (Both Sema and CodeGen tests for this)

Also missing coverage for the changes to -fdefault-calling-conv=

I'm also still wondering whether there's a way to use m68k with a Windows ABI triple (basically, do we need changes in MicrosoftMangle.cpp?)

[aaron.ballman]

Also, it looks like precommit CI found a relevant failure that should be investigated.

[glaubitz]

Any update on this?

[myhsu]

In D149867#4544281, @glaubitz wrote:

Any update on this?

Sorry I was busy on my phd defense (which I passed!) in the past month. I'll get back to this next week.

[myhsu]

• Rebase
• Add more C++ tests for m68k_rtd, both Sema and CodeGen ones
• Addressed other comments

clang/test/Sema/m68k-mrtd.c

4–9 ↗

(On Diff #530089)

I understand what you meant, but the standard only says that using implicit declared identifier is as if extern int ident(); appears in the source code. My interpretation is that in this case since the very source code is compiled with -mrtd, every functions use m68k_rtd rather than C calling convention.

Another example is stdcall: i386 Clang emits a similar warning ("function with no prototype cannot use the stdcall calling convention") when -mrtd is used (to set default CC to stdcall) on the same snippet. Should bar was not called with stdcall under the influence of -mrtd, the aforementioned message would not be emitted in the first place.
i386 GCC also calls bar with stdcall when -mrtd is given (though no warning message is emitted).

45 ↗

(On Diff #530089)

__attribute__((m68k_rtd)) void bar(a, b) int a, b; {} // Should error

Right now Clang only gives a warning about potential behavior change after C23, rather than an error for this kind of syntax. Because it seems like Clang doesn't check this situation against unsupported calling convention. I'm not sure if we should throw the same error as __attribute__((m68k_rtd)) void foo().

I'm also still wondering whether there's a way to use m68k with a Windows ABI triple (basically, do we need changes in MicrosoftMangle.cpp?)

I don't think it's worth it to support m68k with Windows ABI as this combination never existed (and unlikely to happen in the future)

[aaron.ballman]

In D149867#4544489, @myhsu wrote:

Sorry I was busy on my phd defense (which I passed!) in the past month. I'll get back to this next week.

Congratulations!! 🎉

LGTM!

clang/test/Sema/m68k-mrtd.c

4–9 ↗

(On Diff #530089)

Ahhh okay, this makes more sense now -- this matches other ways in which we set the default calling convention, it's just a bit more hidden from view. Here's a more obvious example: https://godbolt.org/z/sboxf83s6

45 ↗

(On Diff #530089)

Right now Clang only gives a warning about potential behavior change after C23, rather than an error for this kind of syntax. Because it seems like Clang doesn't check this situation against unsupported calling convention. I'm not sure if we should throw the same error as attribute((m68k_rtd)) void foo().

It looks like this is an existing Clang bug: https://godbolt.org/z/WcYbcEhde

I don't think it's worth it to support m68k with Windows ABI as this combination never existed (and unlikely to happen in the future)

SGTM!

[jrtc27]

clang/test/CodeGen/mrtd.c

5

Capitalise CHECK prefixes

12

Uh, this shouldn't be defined in ISO C; GCC's using builtin_define_std, so you should only get __mc68000 and __mc68000__ for ISO C, i.e. using DefineStd("mc68000") in libClangBasic and let it add the underscored variants needed

[myhsu]

Addressed comments in test/CodeGen/mrtd.c

clang/test/CodeGen/mrtd.c

12

Thanks for pointing out. I'll put the changes to using DefineStd in a separate patch.

[myhsu]

In D149867#4603853, @aaron.ballman wrote:

In D149867#4544489, @myhsu wrote:

Sorry I was busy on my phd defense (which I passed!) in the past month. I'll get back to this next week.

Congratulations!! 🎉

Thank you!

[glaubitz]

In D149867#4612589, @myhsu wrote:

In D149867#4603853, @aaron.ballman wrote:

In D149867#4544489, @myhsu wrote:

Sorry I was busy on my phd defense (which I passed!) in the past month. I'll get back to this next week.

Congratulations!! 🎉

Thank you!

Oh man, I completely missed that.

Congratulations, Min! That must be a huge relieve!

[glaubitz]

Any news on this? I guess it would be nice to get the remaining m68k patches merged before the Phrabricator shutdown.

[0x59616e]

ping.

[glaubitz]

Maybe these patches need to be reposted to Github as they seem to be ignored here now after the swtich from Phabricator.

[jrtc27]

Posting on GitHub loses all context; please don't do that. Whose review are you looking for? You've addressed my feedback and you had approval from Aaron on the prior version, with the latest version just being a few minor tweaks to address the couple of comments I had left on the approved version.

clang/lib/Basic/Targets/M68k.h

public:
   ArrayRef<TargetInfo::GCCRegAlias> getGCCRegAliases() const override;
   std::string convertConstraint(const char *&Constraint) const override;
   bool validateAsmConstraint(const char *&Name,
                              TargetInfo::ConstraintInfo &info) const override;
   std::optional<std::string> handleAsmEscapedChar(char EscChar) const override;
   std::string_view getClobbers() const override;
   BuiltinVaListKind getBuiltinVaListKind() const override;
   bool setCPU(const std::string &Name) override;
+  CallingConvCheckResult checkCallingConvention(CallingConv CC) const override;
 };
 
 } // namespace targets
 } // namespace clang
 
 #endif

clang/lib/Basic/Targets/M68k.cpp

std::string_view M68kTargetInfo::getClobbers() const {
   // FIXME: Is this really right?
   return "";
 }
 
 TargetInfo::BuiltinVaListKind M68kTargetInfo::getBuiltinVaListKind() const {
   return TargetInfo::VoidPtrBuiltinVaList;
 }
 
+TargetInfo::CallingConvCheckResult
+M68kTargetInfo::checkCallingConvention(CallingConv CC) const {
+  switch (CC) {
+  case CC_C:
+  // The M68k_RTD calling convention.
+  case CC_X86StdCall:
+    return CCCR_OK;
+  default:
+    return TargetInfo::checkCallingConvention(CC);
+  }
+}
 } // namespace targets
 } // namespace clang

clang/lib/CodeGen/CGCall.cpp

 using namespace clang;
 using namespace CodeGen;
 
 /***/
 
 unsigned CodeGenTypes::ClangCallConvToLLVMCallConv(CallingConv CC) {
   switch (CC) {
   default: return llvm::CallingConv::C;
-  case CC_X86StdCall: return llvm::CallingConv::X86_StdCall;
+  case CC_X86StdCall:
+    return CGM.getTriple().getArch() == llvm::Triple::m68k
+               ? llvm::CallingConv::M68k_RTD
+               : llvm::CallingConv::X86_StdCall;
   case CC_X86FastCall: return llvm::CallingConv::X86_FastCall;
   case CC_X86RegCall: return llvm::CallingConv::X86_RegCall;
   case CC_X86ThisCall: return llvm::CallingConv::X86_ThisCall;
   case CC_Win64: return llvm::CallingConv::Win64;
   case CC_X86_64SysV: return llvm::CallingConv::X86_64_SysV;
   case CC_AAPCS: return llvm::CallingConv::ARM_AAPCS;
   case CC_AAPCS_VFP: return llvm::CallingConv::ARM_AAPCS_VFP;
   case CC_IntelOclBicc: return llvm::CallingConv::Intel_OCL_BI;

clang/lib/Frontend/CompilerInvocation.cpp

if (Args.getLastArg(OPT_cl_strict_aliasing) &&
       (LangOpts.getOpenCLCompatibleVersion() > 100))
     Diags.Report(diag::warn_option_invalid_ocl_version)
         << LangOpts.getOpenCLVersionString()
         << Args.getLastArg(OPT_cl_strict_aliasing)->getAsString(Args);
 
   if (Arg *A = Args.getLastArg(OPT_fdefault_calling_conv_EQ)) {
     auto DefaultCC = LangOpts.getDefaultCallingConv();
 
-    bool emitError = (DefaultCC == LangOptions::DCC_FastCall ||
-                      DefaultCC == LangOptions::DCC_StdCall) &&
-                     Arch != llvm::Triple::x86;
+    bool emitError =
+        DefaultCC == LangOptions::DCC_FastCall && Arch != llvm::Triple::x86;
+    emitError |= DefaultCC == LangOptions::DCC_StdCall &&
+                 Arch != llvm::Triple::m68k && Arch != llvm::Triple::x86;
     emitError |= (DefaultCC == LangOptions::DCC_VectorCall ||
                   DefaultCC == LangOptions::DCC_RegCall) &&
                  !T.isX86();
     if (emitError)
       Diags.Report(diag::err_drv_argument_not_allowed_with)
           << A->getSpelling() << T.getTriple();
   }
 
#undef LANG_OPTION_WITH_MARSHALLING
   llvm::sort(Opts.ModuleFeatures);
 
   // -mrtd option
   if (Arg *A = Args.getLastArg(OPT_mrtd)) {
     if (Opts.getDefaultCallingConv() != LangOptions::DCC_None)
       Diags.Report(diag::err_drv_argument_not_allowed_with)
           << A->getSpelling() << "-fdefault-calling-conv";
     else {
-      if (T.getArch() != llvm::Triple::x86)
+      if (T.getArch() != llvm::Triple::x86 && T.getArch() != llvm::Triple::m68k)
         Diags.Report(diag::err_drv_argument_not_allowed_with)
             << A->getSpelling() << T.getTriple();
       else
         Opts.setDefaultCallingConv(LangOptions::DCC_StdCall);
     }
   }
 
   // Check if -fopenmp is specified and set default version to 5.0.

clang/test/CodeGen/mrtd.c

-// RUN: %clang_cc1 -mrtd -triple i386-unknown-unknown -std=c89 -emit-llvm -o - %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -mrtd -triple i386-unknown-unknown -std=c89 -emit-llvm -o - %s 2>&1 | FileCheck --check-prefixes=CHECK,X86 %s
+// RUN: %clang_cc1 -mrtd -triple m68k-unknown-unknown -std=c89 -emit-llvm -o - %s 2>&1 | FileCheck --check-prefixes=CHECK,M68k %s
 
-// CHECK: mrtd.c:10:3: warning: function with no prototype cannot use the stdcall calling convention
+// CHECK: mrtd.c:13:3: warning: function with no prototype cannot use the stdcall calling convention
 
 void baz(int arg);
 
-// CHECK: define{{.*}} x86_stdcallcc void @foo(i32 noundef %arg) [[NUW:#[0-9]+]]
+// X86: define{{.*}} x86_stdcallcc void @foo(i32 noundef %arg) [[NUW:#[0-9]+]]
+// M68k: define{{.*}} m68k_rtdcc void @foo(i32 noundef %arg)
 void foo(int arg) {
-// CHECK: call x86_stdcallcc i32 @bar(
+// X86: call x86_stdcallcc i32 @bar(
+// M68k: call m68k_rtdcc i32 @bar(
   bar(arg);
-// CHECK: call x86_stdcallcc void @baz(i32
+// X86: call x86_stdcallcc void @baz(i32
+// M68k: call m68k_rtdcc void @baz(i32
   baz(arg);
 }
 
-// CHECK: declare x86_stdcallcc i32 @bar(...)
+// X86: declare x86_stdcallcc i32 @bar(...)
+// M68k: declare m68k_rtdcc i32 @bar(...)
 
-// CHECK: declare x86_stdcallcc void @baz(i32 noundef)
+// X86: declare x86_stdcallcc void @baz(i32 noundef)
+// M68k: declare m68k_rtdcc void @baz(i32 noundef)
 
 void qux(int arg, ...) { }
 // CHECK: define{{.*}} void @qux(i32 noundef %arg, ...)
 
 void quux(int a1, int a2, int a3) {
   qux(a1, a2, a3);
 }
-// CHECK-LABEL: define{{.*}} x86_stdcallcc void @quux
+// X86-LABEL: define{{.*}} x86_stdcallcc void @quux
+// M68k-LABEL: define{{.*}} m68k_rtdcc void @quux
 // CHECK: call void (i32, ...) @qux
 
-// CHECK: attributes [[NUW]] = { noinline nounwind{{.*}} }
+// X86: attributes [[NUW]] = { noinline nounwind{{.*}} }

clang/test/Sema/mrtd.c

 // RUN: %clang_cc1 -DMRTD -mrtd -triple i386-unknown-unknown -verify %s
+// RUN: %clang_cc1 -DMRTD -mrtd -triple m68k-unknown-unknown -verify %s
 // RUN: %clang_cc1 -triple i386-unknown-unknown -verify %s
+// RUN: %clang_cc1 -triple m68k-unknown-unknown -verify %s
 
 #ifndef MRTD
 // expected-note@+5 {{previous declaration is here}}
 // expected-error@+5 {{function declared 'stdcall' here was previously declared without calling convention}}
 // expected-note@+5 {{previous declaration is here}}
 // expected-error@+5 {{function declared 'stdcall' here was previously declared without calling convention}}
 #endif
 void nonvariadic1(int a, int b, int c);