This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by myhsu on May 4 2023, 9:31 AM.

Details

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.

Diff Detail

Event Timeline

myhsu created this revision.May 4 2023, 9:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2023, 9:31 AM
Herald added a subscriber: pengfei. · View Herald Transcript
myhsu requested review of this revision.May 4 2023, 9:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2023, 9:31 AM
0x59616e added inline comments.May 4 2023, 10:23 PM
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 added inline comments.May 4 2023, 10:26 PM
clang/test/CodeGen/mrtd.c
9

I guess this involves more work ?

myhsu added inline comments.May 5 2023, 9:51 AM
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 updated this revision to Diff 519982.May 5 2023, 2:52 PM

Introduce m68k_rtdcc the textual LLVM IR designation for M68k_RTD

myhsu updated this revision to Diff 519983.May 5 2023, 2:56 PM

Minor updates. NFC.

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

jrtc27 added a subscriber: jrtc27.May 7 2023, 6:56 AM

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 added a comment.May 8 2023, 2:47 PM

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.

0x59616e accepted this revision.EditedMay 31 2023, 10:33 PM

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 ?

This revision is now accepted and ready to land.May 31 2023, 10:33 PM
jrtc27 requested changes to this revision.EditedMay 31 2023, 10:54 PM

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.

This revision now requires changes to proceed.May 31 2023, 10:54 PM

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?

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 updated this revision to Diff 530089.Jun 9 2023, 2:52 PM
myhsu retitled this revision from [M68k] Add Clang support for the new M68k_RTD CC to [Clang][M68k] Add Clang support for the new M68k_RTD CC.
myhsu edited the summary of this revision. (Show Details)
myhsu set the repository for this revision to rG LLVM Github Monorepo.

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

myhsu marked 3 inline comments as done.
myhsu added inline comments.
clang/test/CodeGen/mrtd.c
4

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

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 updated this revision to Diff 532423.Jun 17 2023, 2:22 PM
myhsu marked 5 inline comments as done.
myhsu edited the summary of this revision. (Show Details)
  • 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 added inline comments.Jun 17 2023, 2:22 PM
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.

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?)

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

Any update on this?

myhsu added a comment.Jul 29 2023, 1:30 PM

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 updated this revision to Diff 551797.EditedAug 19 2023, 4:08 PM
myhsu marked 4 inline comments as done.
  • 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 accepted this revision.Aug 21 2023, 8:55 AM

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 added inline comments.Aug 21 2023, 9:11 AM
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 updated this revision to Diff 552596.Aug 22 2023, 11:16 PM
myhsu marked 2 inline comments as done.

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.

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!

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!

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

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

jrtc27 added a comment.Oct 5 2023, 9:03 AM

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.

0x59616e accepted this revision.Oct 5 2023, 9:09 AM
This revision was not accepted when it landed; it landed in state Needs Review.Oct 15 2023, 4:15 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.