This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Add proper target checks for SwiftAsync convention.
AbandonedPublic

Authored by varungandhi-apple on Feb 16 2021, 10:49 AM.

Details

Summary

Previously, we erroneously claimed that all targets were supported.
However, at the moment, only x86_64 and ARM are known to be working.

Diff Detail

Event Timeline

varungandhi-apple requested review of this revision.Feb 16 2021, 10:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2021, 10:49 AM
aschwaighofer accepted this revision.Feb 16 2021, 11:32 AM
aschwaighofer added a subscriber: aschwaighofer.

Looks good to me

This revision is now accepted and ready to land.Feb 16 2021, 11:32 AM

The TargetInfo classes are already target-architecture-specific, so it's somewhat strange for them all to funnel to a single function that then immediately switches on the target architecture.

The TargetInfo classes are already target-architecture-specific, so it's somewhat strange for them all to funnel to a single function that then immediately switches on the target architecture.

IMO ideally, the whole thing would be stored as a table of Target x CallingConvention. Each entry of the table would have a CallingConvCheckResult. That way, you can quickly figure out both:

  • which targets is a calling convention supported for
  • what calling conventions are supported for a particular target

Certainly, that's not the state of the code right now. So I do agree with your point that it looks a bit strange. However, given that things are somewhat in flux for CC_SwiftAsync, I think it is valuable to centralize the information for it in one place instead of spreading it out over multiple files.

varungandhi-apple abandoned this revision.Feb 17 2021, 3:22 PM

I've updated the earlier patch https://reviews.llvm.org/D95561 to include this change, so that we don't have an in-between state where we erroneously claim that we support a bunch of targets which we don't actually support.

The TargetInfo classes are already target-architecture-specific, so it's somewhat strange for them all to funnel to a single function that then immediately switches on the target architecture.

IMO ideally, the whole thing would be stored as a table of Target x CallingConvention. Each entry of the table would have a CallingConvCheckResult. That way, you can quickly figure out both:

  • which targets is a calling convention supported for
  • what calling conventions are supported for a particular target

Certainly, that's not the state of the code right now. So I do agree with your point that it looks a bit strange. However, given that things are somewhat in flux for CC_SwiftAsync, I think it is valuable to centralize the information for it in one place instead of spreading it out over multiple files.

I don't see why. I can't really imagine adding a second dimension to the decision here — there's no reason to ever make this OS-dependent, for example — and so the only likely evolution is adding support for a new architecture, which means adding another case to your centralized function instead of just changing how the CC is handled in the target-specific function. And for this patch, just implementing the new case as return CCCR_Error seems much more obvious than calling a centralized function which just switches out again.