[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

jhibbits added a comment. Not in 9.0, but I will try to push for it in 9.0.1. To others, for posterity, I pushed *only* the SPE subset, not the 8548 CPU component. That will be a separate commit. I wanted it to be more than just a dummy stub, so that part will be a separate commit. The part

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

vit9696 added a comment. @jhibbits, thank you for merging. Will we have this in LLVM 9.0? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D49754/new/ https://reviews.llvm.org/D49754 ___ cfe-commits mailing list cfe-commi

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rL371066: Add -m(no)-spe to clang (authored by jhibbits, committed by ). Herald added a project: LLVM. Herald added a subscr

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

jhibbits requested review of this revision. jhibbits added a comment. Should've marked it as need review earlier. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D49754/new/ https://reviews.llvm.org/D49754 ___ cfe-commi

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

jhibbits updated this revision to Diff 207214. jhibbits added a comment. Made '8548' CPU designation just a stub, to be filled out later. I added it just for parity with GCC. The 8548 CPU, for GCC, also sets the __NO_LWSYNC__ macro, which doesn't belong with the SPE change, so will have to be re

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

vit9696 added a comment. Not sure whether I understood you, do not you already have the common switch by feature, named spe, in Features["spe"] line for that? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D49754/new/ https://reviews.llvm.org/D49754 ___

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

jhibbits added a comment. Actually, I'm not yet ready to commit this. I want to enforce the 8548 -> e500 processor model before I call this ready. How do I do that with the mcpu? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D49754/new/ https://reviews.llvm.org

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

jhibbits added a comment. I'll commit it tonight. Was going to last night, but ran into a clang test failure, that turned out to be a long-standing failure with FreeBSD/powerpc64, not a problem with my change. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D49754

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

vit9696 added a comment. Right, ok. I have been testing this for quite some time now, including maths, and so far had no issues. Can this get merged into 9.0? I do not believe there are enough obstacles to postpone it any further. Thanks! Repository: rC Clang CHANGES SINCE LAST ACTION htt

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

jhibbits marked an inline comment as done. jhibbits added inline comments. Comment at: lib/Basic/Targets/PPC.cpp:318 + Features["spe"] = llvm::StringSwitch(CPU) +.Case("e500", true) +.Case("8548", true) vit9696 wro

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

vit9696 added a comment. @nemanjai, sorry, under merging I meant committing into llvm upstream. Comment at: lib/Basic/Targets/PPC.cpp:318 + Features["spe"] = llvm::StringSwitch(CPU) +.Case("e500", true) +.Case("8548", true) -

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

nemanjai added a comment. In D49754#1402790 , @vit9696 wrote: > This is a series of patches, which I believe should merged altogether. > Currently the following patches are relevant: No, please don't merge them together. It is much more manageable for r

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

nemanjai accepted this revision. nemanjai added a comment. This revision is now accepted and ready to land. Please, no more patches without context. This one was actually easy to review without context and the comments are minor, so I'm fine with these being addressed on the commit. =

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

vit9696 added a comment. This is a series of patches, which I believe should merged altogether. Currently the following patches are relevant: - https://reviews.llvm.org/D49754 - https://reviews.llvm.org/D54409 - https://reviews.llvm.org/D54583 - https://reviews.llvm.org/D56703 The patches are i

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

nemanjai added a comment. There is a long series of comments in this patch and I am not clear at this point on whether this patch breaks anything or it is fine. Could you please `Request Changes` if this patch is broken or approve if it is fine? Repository: rC Clang CHANGES SINCE LAST ACTIO

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

vit9696 added a comment. @jhibbits, @kthomsen, it appears that current patchset has issues when handling && on me. I have it applied over llvm 8.0.0 rc2, and the following code returns 0 to me with -O2 and below: #include #define FEQUAL(x,y) (((x) - (y)) < 0.01) // could put fabs if

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

jhibbits updated this revision to Diff 185487. jhibbits added a comment. Herald added a project: clang. Add feedback from @vit9696, and VAARG fix from @kthomsen. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D49754/new/ https://reviews.llvm.org/D49754 Files: in

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

jhibbits added a comment. @vit9696 sure thing. We'll need to get all these patches in together before any are actually useful. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D49754/new/ https://reviews.llvm.org/D49754 ___

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

vit9696 added a subscriber: hfinkel. vit9696 added a comment. That's pretty good. Do you think it is possible to land it in 8.0 release? @hfinkel? @jhibbits could you please include this change too: https://reviews.llvm.org/D49754#1364865? We would prefer to continue using Linux target to be a

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

jhibbits added a comment. Hi Kei, that's fantastic! There's one more thing to add to this, which is to predefine __NO_FPRS__, and it should be a good replacement for gcc for 90+% of cases. I'll add your changes and this, and resubmit this review. Thanks for all your help! Repository: rC C

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

kthomsen added a comment. With this modification for SPE in VAARG, I was now able to compile all OS-9 libraries for SPE and tested them with whetstone. The results of the whetstone are the same like with a real FPU and they are correctly shown with printf. Also the performance of CLANG is about

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

vit9696 added a comment. Thanks for pointing it out. You could use hasFeature from there during construction: return SetCGInfo( new PPC32TargetCodeGenInfo(Types, CodeGenOpts.FloatABI == "soft" || getTarget().hasFeature("spe"))); It works for me, but probably a separate argument is

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

kthomsen added a comment. The desired function for this va_arg is not in lib/Target/PowerPC/*.cpp, it is in tools/clang/lib/CodeGen/TargetInfo.cpp , a little bit unexpected to me. PPC32_SVR4_ABIInfo::EmitVAArg() is doing the va_arg handling. For testing, I have added a hasSPE = true and treat th

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

vit9696 added a comment. Right, I noticed the same thing yesterday. There is an override calling LowerVAARG for 64-bit integers, yet that is not a thing that does lowering for all the rest. I believe it is derived somewhere from td calling conventions. I will check it out later this evening and

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

kthomsen added a comment. @vit9696 I'm working since 3 days on that issue, and found nothing... PPCISelLowering.cpp has 2 functions: LowerVASTART() and LowerVAARG(). LowerVASTART is correctly called (store the GPR to the internal va_list structure), but LowerVAARG is never called and I don't un

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

vit9696 added a comment. @jhibbits it appears that va_list is not functional with SPE (va_arg returns garbage for double and stuff like printf is not functional). Is this expected or I miss a patch? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D49754/new/ https

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

vit9696 added a comment. Justin, I am currently testing the latest patches, yet so far it looks very good. Thanks. I rechecked GCC, and it seems that it forces 64-bit long double for SPE regardless of the target. Could you please force that in LLVM as well? While imperfect, I currently changed

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

jhibbits added a comment. Hi @vit9696, thanks for that, it was a straightforward fix. I'll post an update shortly for D54583 , if arcanist cooperates. The short of it is I need two indices for arguments, since one is for logical arguments the other is for phys

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

vit9696 added a comment. You are right, had to modify it like this to get the crash with FreeBSD triple: void f1(long double v, void *a) { } void f2(void* v, __builtin_va_list arg) { f1(__builtin_va_arg(arg, long double), 0); } Repository: rC Clang CHANGES SINCE LAST ACTIO

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

jhibbits added a comment. Hi @vit9696 it does crash with the linux target (powerpc-gnu-linux), but is fine with powerpc-gnu-freebsd. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D49754/new/ https://reviews.llvm.org/D49754 _

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

vit9696 added a comment. Actually I am not sure about Linux, since this is bare metal, and I just used what fited. However, it does not look like 128-bit or 64-bit long doubles are related. I retried to compile the test case with powerpc-gnu-freebsd target (and even made a compile-time assertio

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

jhibbits added a comment. Hi @vit9696 , This looks to be caused by using 128-bit long double on the platform. Does linux really use 128-bit long doubles on ppc32? FreeBSD uses 64-bit long double, so compiling that with '-target powerpc-gnu-freebsd' works fine. I'm not sure how to handle the

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

vit9696 added a comment. @jhibbits sorry for not answering earlier, I was occupied with NY holidays, and then had a lot of stuff on the road. Trying to sync to your latest changes, I merged the updated https://reviews.llvm.org/D54583 in my local copy with your fixes to libcall expansion. From

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

jhibbits added a comment. Hi vit, I found what's going on with the "Cannot Select" error. divdc3.c contains code that gets lowered to a libcall. However this lowering doesn't go further to lower down to the legal operations permitted in this target. The debug snippet is: Legalizing: t38: f6

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

glaubitz added a comment. In D49754#1339161 , @vit9696 wrote: > Ok, I found the fix for the first crash that landed in 8.0 trunk. It works > fine for me if backported to 7.0.1: > https://reviews.llvm.org/D50461 That's awesome, thanks so much for fixing

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

vit9696 added a comment. Ok, I found the fix for the first crash that landed in 8.0 trunk. It works fine for me if backported to 7.0.1: https://reviews.llvm.org/D50461 Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D49754/new/ https://reviews.llvm.org/D49754 __

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

glaubitz added a comment. In D49754#1323642 , @vit9696 wrote: > Might the first crash from https://reviews.llvm.org/D49754#1183753 reproduce > for you or perhaps you have already bisected to trunk to figure out the > changest that fixes it? Yes, it doe

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

vit9696 added a comment. In D49754#1321078 , @glaubitz wrote: > I have applied this patch to the llvm-toolchain-7 package in Debian and did > not see any regressions on x86_64 or 32-Bit PowerPC. Additionally, I have > included the patches from https://re

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

glaubitz added a comment. I have applied this patch to the llvm-toolchain-7 package in Debian and did not see any regressions on x86_64 or 32-Bit PowerPC. Additionally, I have included the patches from https://reviews.llvm.org/D54409 and https://reviews.llvm.org/D54583 saw no regressions on x86

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

jhibbits added a comment. Hi Vit, I'm unable to reproduce the previous crash with clang8, so for now assuming it's fixed by other events. If that turns out not to be the case, I can revisit it. I am able to reproduce this crash though, so am taking a look. I'm' not quite sure how to interpr

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

vit9696 added a comment. Thanks for the fix. I made a quick check of the mentioned patch, and it looks like it does solve the issue. However, besides the previous crash, which remains unfixed as of 7.0.1rc2, there is another instruction selection failure crash that may be caused by the change.

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

jhibbits added a comment. Hi Vit, The register spilling bug is being addressed in https://reviews.llvm.org/D54409 now. Repository: rC Clang https://reviews.llvm.org/D49754 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llv

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

jhibbits added a comment. Hi Vit, Thanks for the feedback. I can add the -mspe=yes/no, that shouldn't be hard. I didn't include it because it's been deprecated by GCC already as well. I can add the -mcpu=8548 option as well. I use -mcpu=8540 on FreeBSD for the powerpcspe target anyway (GCC

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

vit9696 added a comment. Hello, Thank you for working this. I tried the change and have a couple of suggestions: 1. -mspe option in GCC works like -mspe=yes or -mspe=no. While it does make sense to have it the way you did (-mno-spe and -mspe) it would be nice to have at least have an alias for

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

jhibbits created this revision. Herald added subscribers: cfe-commits, kbarton, nemanjai. r337347 added support for the Signal Processing Engine (SPE) to LLVM. This follows that up with the clang side. Repository: rC Clang https://reviews.llvm.org/D49754 Files: include/clang/Driver/Options