Page MenuHomePhabricator

[Preprocessor] Implement __is_target_{arch|vendor|os|environment} function-like builtin macros
ClosedPublic

Authored by arphaman on Dec 11 2017, 12:50 PM.

Details

Summary

This patch implements the __is_target_arch, __is_target_vendor, __is_target_os, and __is_target_environment Clang preprocessor extensions that were proposed by @compnerd in Bob's cfe-dev post: http://lists.llvm.org/pipermail/cfe-dev/2017-November/056166.html.

These macros can be used to examine the components of the target triple at compile time. A`has_builtin(is_target_???)` preprocessor check can be used to check for their availability.

Diff Detail

Repository
rC Clang

Event Timeline

arphaman created this revision.Dec 11 2017, 12:50 PM
compnerd added inline comments.Dec 11 2017, 9:25 PM
lib/Lex/PPMacroExpansion.cpp
1974

Hmm, the one thing to consider here is the canonicalized vs spelt target. e.g. armv7-windows will map to thumbv7-unknown-windows-msvc.

test/Preprocessor/is_target.c
8 ↗(On Diff #126421)

Can you use mismatching arch instead? This helps differentiate between the invalid arch case below as apposed to the condition being false. Similar for the other warnings.

arphaman marked an inline comment as done.Dec 12 2017, 10:42 AM
arphaman added inline comments.
lib/Lex/PPMacroExpansion.cpp
1974

I think it's ok to only allow "thumb" check to succeed instead of "arm", otherwise how would we differentiate between the two? However, we should take the sub arch into account, so when arch is "thumbv7", these checks should succeed:

__is_target_arch(thumb)
__is_target_arch(thumbv7)

but this one should fail:

__is_target_arch(thumbv6)

I fixed this in the updated patch.

arphaman updated this revision to Diff 126581.Dec 12 2017, 10:43 AM
  • Change error message.
  • Take sub arch into account.
compnerd accepted this revision.Dec 13 2017, 10:58 AM

It would be good to straighten out the corner case of the canonicalized vs specified triple before merging this as that would make it harder to change.

Minor nit with the style, I'm not too fond of the longer lambdas being inlined, having a separate function being passed instead would be easier to read I think.

lib/Lex/PPMacroExpansion.cpp
1974

I'm considering the scenario where the user specifies -target armv7-windows and the __is_target_arch(thumb) passes but __is_target_arch(arm) fails. Is that really what people would expect?

test/Preprocessor/is_target_os_darwin.c
22 ↗(On Diff #126581)

Is this supposed to be within the MAC clause?

This revision is now accepted and ready to land.Dec 13 2017, 10:58 AM
arphaman added inline comments.Dec 13 2017, 11:41 AM
lib/Lex/PPMacroExpansion.cpp
1974

I suppose __is_target_arch(arm) should probably work in that case too, especially seeing that we define the ARM macros already. The users would just have to differentiate between thumb and non-thumb by checking if __is_target_arch(thumb) succeeds too.

arphaman marked an inline comment as done.Dec 13 2017, 11:42 AM
arphaman added inline comments.
test/Preprocessor/is_target_os_darwin.c
22 ↗(On Diff #126581)

Yep, this should not succeed on Mac.

arphaman updated this revision to Diff 126856.Dec 13 2017, 3:39 PM
arphaman marked an inline comment as done.
  • Use separate functions for checks.
  • "ARM" should match "thumb" arch too.
This revision was automatically updated to reflect the committed changes.

I'm concerned here about the check for the names as written vs. the canonical names. @compnerd pointed out one specific case with armv7, and I see that you've got special handling for "darwin", but I think there are more. What about "arm64" vs. the canonical "aarch64"? Look through the triple parsing code in Triple.cpp and I'm pretty sure you'll find more. I had been thinking about using the canonical names. However, that's not ideal either because the canonical names intentionally exclude suffixes that some users may want to distinguish (e.g., armv7 vs armv7k).

I'm concerned here about the check for the names as written vs. the canonical names. @compnerd pointed out one specific case with armv7, and I see that you've got special handling for "darwin", but I think there are more. What about "arm64" vs. the canonical "aarch64"? Look through the triple parsing code in Triple.cpp and I'm pretty sure you'll find more. I had been thinking about using the canonical names. However, that's not ideal either because the canonical names intentionally exclude suffixes that some users may want to distinguish (e.g., armv7 vs armv7k).

Ah, you're right. I should just check the arch instead of the arch name and the subarch too (for the "arm64" vs "aarch64"). I fixed that in r320853. I believe that other similar arch cases are already handled when there's no subarch.
I think that the other component that's troubling is environment because of the version number. I fixed that in r320854.

The "armv7" and "armv7k" will have to stay separate I think. If "armv7k" is the target arch then the user would either have to check for __is_target_arch(arm) or __is_target_arch(armv7k), and __is_target_arch(armv7) will fail. That seems reasonable to me though.