REPOSITORY
rL LLVM
================
Comment at: lib/Basic/Targets.cpp:723
@@ -722,1 +722,3 @@
bool HasP8Crypto;
+ bool IsPwr7Up;
+ bool IsPwr8Up;
----------------
We don't need the 'Up' in the names here; I think we can generally assume that
newer cores will provide backward compatibility, and only when they don't would
we need some kind of HasP8Only feature.
================
Comment at: lib/Basic/Targets.cpp:725
@@ -723,1 +724,3 @@
+ bool IsPwr8Up;
+ bool Is64Bit;
----------------
Please remove this. We can get at the current pointer size from generic code.
================
Comment at: lib/Basic/Targets.cpp:1217
@@ -1204,1 +1216,3 @@
+ .Case("IsISA207Up", IsPwr8Up)
+ .Case("Is64Bit", Is64Bit)
.Default(false);
----------------
nemanjai wrote:
> nemanjai wrote:
> > wschmidt wrote:
> > > mmm, even if we keep this approach, I'm not sure about the Is64Bit one.
> > > Surely these aren't the only built-ins that only work for 64-bit. If
> > > you're going to add this it needs to be complete and consistent, which is
> > > far more than what you're going for here. My inclination is to leave it
> > > out and consider whether a full patch for this is sensible down the road.
> > I will investigate whether other 64-bit only builtins provide any
> > diagnostics if used in code that is compiled at 32-bit. Thanks for bringing
> > this up.
> OK, as it turns out, this seems to largely be ignored by other targets (and
> we do not seem to have any builtins implemented that are available on 64-bit
> only.
> I personally feel that diagnosing this is better than just letting the back
> end crash.
> Example from Arm:
>
> ```
> $ cat t.c
> void test_clrex() {
> __builtin_arm_clrex();
> }
> $ clang t.c --target=arm-apple-ios7.0 -c
> fatal error: error in backend: Cannot select: intrinsic %llvm.arm.clrex
> clang: error: clang frontend command failed with exit code 70 (use -v to see
> invocation)
> clang version 3.7.0 (trunk 231831) (llvm/trunk 231828:231843M)
> Target: arm-apple-ios7.0
> Thread model: posix
> clang: note: diagnostic msg: PLEASE submit a bug report to
> http://llvm.org/bugs/ and include the crash backtrace, preprocessed source,
> and associated run script.
> clang: note: diagnostic msg:
> ********************
>
> PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
> Preprocessed source(s) and associated run script(s) are located at:
> clang: note: diagnostic msg: /tmp/t-4a719a.c
> clang: note: diagnostic msg: /tmp/t-4a719a.sh
> clang: note: diagnostic msg:
>
> ********************
>
> ```
> Similar example with this changeset:
>
> ```
> $ cat a.c
> long long test_divde(void) {
> return __builtin_divde(74LL, 32LL);
> }
> $ clang a.c -c --target=powerpc-unknown-unknown -mcpu=pwr7
> a.c:2:10: error: This builtin is only available on 64-bit targets
> return __builtin_divde(74LL, 32LL);
> ^
> 1 error generated.
>
> ```
> I personally feel that a diagnostic identifying the problem is more helpful
> to the user than a message that the back end crashed and suggestion to open a
> bug report.
I agree, we should generate an error in the frontend instead of just crashing
in the backend.
================
Comment at: lib/CodeGen/CGBuiltin.cpp:6390
@@ +6389,3 @@
+ BuiltinID == PPC::BI__builtin_bpermd;
+ if(!getTarget().hasFeature("IsPwr7Up")) {
+ CGM.Error(E->getExprLoc(),
----------------
nemanjai wrote:
> wschmidt wrote:
> > I'd prefer that the conditions you've encoded as features just be tested
> > here, rather than using the feature mechanism, but this is a call for
> > people with more experience with this than me.
> I am certainly not opposed to this. Doing so would negate the need for the
> "fake features" from above. However, the reason for doing it this way was in
> anticipation of any future CPU's that are a superset of Power7...
> Say we implement 5 builtins that are only available on Power7. Then another
> 17 that are available on Power8. We then get CPU X which is a superset of
> both. We would need to track down all the places where we check for the
> correct CPU. Using this approach puts this check in one place.
> However, all this is just to explain why I did it this way. If you (or other
> interested parties) feel that I shouldn't, I'd be happy to change it.
I have a weak preference for keeping the cpu -> feature logic localized to the
target code, so I think this general scheme is good.
http://reviews.llvm.org/D8398
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
