- User Since
- Sep 8 2014, 1:26 PM (314 w, 5 d)
Jul 31 2020
Jul 21 2020
As mentioned before, I can only seem to reproduce this running natively on powerpcspe; running the test on powerpc64, even without this patch, succeeds. This is extremely bizarre, but this fix gets the native build to produce the exact same text as running the test on powerpc64.
Simplify the test a little.
Jul 10 2020
Funny thing, I'm able to reproduce it *only* while running natively on a powerpcspe based device, not on any other device I've tested. I'll try to reduce the testcase even further, since it really is unwieldy.
Ping again? Since it's a trivial change, and isolated at that, I'm not opposed to post-commit review instead.
Looks fine to me.
Jun 26 2020
Jun 25 2020
Jun 24 2020
Jun 6 2020
May 29 2020
May 26 2020
May 12 2020
Apr 30 2020
Address feedback from @shchenz. Much better!
Apr 22 2020
Apr 21 2020
Apr 8 2020
Apr 7 2020
@ZarkoCA I think someone else should also review this, so added @nemanjai as a potential reviewer. He might have more insight to the code in question. I'd like to see this go in soon, though, so that it gets into 11, and we can use it in FreeBSD.
Apr 6 2020
Apr 3 2020
Code looks fine, and others have tested it. Good to see a reversion of the ABI to expected for GCC compatibility on the BSDs.
Mar 26 2020
Mar 25 2020
Can you fix the title of this? "Fix PR45297" is not very descriptive unless someone also reviews the PR.
Mar 24 2020
Update tests. Updated other tests in the same file that looked suspect as well.
Mar 23 2020
Feb 4 2020
Jan 22 2020
Thanks for doing this! It's been on my TODO list for a little while already for SPE, but I never got around to it.
Jan 14 2020
Jan 13 2020
Jan 10 2020
Fine with me. I just wanted -target powerpcspe-... to work with D72014.
Jan 8 2020
Jan 7 2020
Jan 6 2020
Not happy, but won't hold it up.
Jan 5 2020
Any more feedback on this?
Jan 2 2020
@Jim Yeah, I know. I normally do fix the commit message, but forgot before pushing this one. I'll have to check, but I thought arc had a way to update the commit message when generating the review.
Committed as 2c4620ad57b
Address comments. As part of this I ran 'update_llc_test_checks.py' on the
whole spe.ll file. I'm not sure if that's necessary for this, but I included
the output anyway. If it's better as a separate change I can do that.
Dec 31 2019
Add triple unit test. I chose freebsd as the OS since that's my environment.
Dec 30 2019
Reuse the SPE feature test preprocessor test check instead of duplicating it.
powerpcspe-* is exactly equivalent to the -mspe command line argument.
Fix a test typo made at the last minute.
Ping on this? I've been using this patch for quite some time now, and really want to get it in before 10.
Dec 27 2019
The conflict I see is the other patch uses VSX for the operations, but this uses instructions that have been around as far back as the PPC970.
Dec 19 2019
@kthomsen do you have any tests we can massage for this?
Dec 18 2019
Updated diff after clang-format. I actually had run git-clang-format on it
before submitting, but forgot to amend it to the change before submitting
Update diff. It would help if I actually compiled when rebasing...
Dec 17 2019
Dec 16 2019
Thanks @Jim, yes the code was copied from a comment in another review and I didn't run clang-format on it. Thanks for pointing out 'git-clang-format', I didn't know about that before.
Apply style fixes.
Dec 13 2019
Add tests, taken from the C test in comments in D54583.
Dec 10 2019
@Bdragon28 we might need to use -mlongcall for modules, if we aren't already. This should(?) force the compiler to generate the appropriate long-distance code sequences. I'm not sure, though, if clang supports this yet.
Dec 5 2019
Hi Stefan, thanks for the review. You're right this and the other two reviews (D69483, D69484) need tests. @kthomsen created these, I'm trying to marshall them in. He has some tests, but not in a state that's committable yet, and is working on that/looking for help to create reduced cases.
I might object less to a _rec suffix. However, you would still need to understand what "record form" means, which requires understanding of the ISA. Why not simply add a comment block to the top of PPCInstrInfo.td, describing things like this? I have a feeling other newbies would have similar questions/concerns/complaints, and they can't all be addressed by renaming everything.
Dec 3 2019
I personally think the status quo is easy to read. The definitions look like the instructions themselves, and the added verbosity seems very unnecessary. I don't know about others, but I consider people working in the PPCInstr*.td files to be either knowing exactly what they're looking for (grepping), or have a sufficient handle on the ISA that the existing notation is clear. At most I could see changing the 'o' suffix to '_o', which might help newcomers familiar with '_' notation to indicate a diminutive or subscript, but '_record' is very verbose, and as @nemanjai points out, can make some text very ugly.
I still hold pretty strongly that this change is a more negative impact than positive. As shown in the tests, it's very easy to read the generated statements as "FOO-dot", and search for what 'foo.' actually is. The only difficult part is the '*8' vs '*4', since you can't search for that, and need to remember to drop the number, which isn't difficult after the first time.
Dec 2 2019
When discussing optimizations related to "ADD-o", if it's "ADDO" I'd call it "ADD-O", but if it's "ADDo" I'd call it "ADD-dot", since all record forms are referred to as 'dot's, even in all the documentation I've seen. For instance, immediately above this text box is the "rlwinm_rldicl_to_andi.mir" test, with the change of "ANDIo8..." to "ANDIr8".... I read that instinctively as "ANDI-dot-8", whereas "ANDIr8" looks like "ANDI-R-8" which doesn't have a corresponding instruction in the ISA. Additionally, the asm string in PPCInstrInfo.td is "andi. ...", which would make someone question what the 'r' is for, when it doesn't even exist in the instruction. Seeing a 'ANDIo' alongside "andi." someone will easily see the 'o' stands for the '.'.
I second @nemanjai, the lowercase 'o' suffix stands out to me as a clear indicator of it being punctuation, not a letter in the instruction. I would much rather see ANDIo than ANDIr, the 'r' just doesn't look right when reading over it. My vote is to reject. It looks like a lot of churn without much benefit.
Nov 28 2019
Without this or D70570, secure-PLT does not work. the @plt relocation annotations cause GNU ld to force BSS-PLT instead.
Nov 24 2019
Nov 21 2019
Nov 16 2019
Nov 11 2019
@vit9696 mind reviewing again with the added tests?
Nov 9 2019
Add clang-translation tests for e500 and 8548 CPU definitions.
Oct 29 2019
@kthomsen you can use 'clang -emit-llvm' to emit LLVM IR from your C test cases. There are reducer passes that you can run on that as well, to reduce the test case to the smallest needed, but I can't recall what they are.
Oct 28 2019
Thanks, @kthomsen. Can you provide a LLVM IR test case for this? And test cases for the other two reviews I added you on, since you wrote the patches initially?
Oct 27 2019
Don't double-check 8548 CPU for setting LLVM CPU type.
Oct 25 2019
Make 8548 actually denote e500 LLVM target, add SPE checks to 8548 preprocessor test.
Oct 24 2019
Oct 23 2019
@vit9696 The only thing GCC defines for mcpu=8548 is NO_LWSYNC, the SPE-specific #defines are from -mspe. That said, since I explicitly do enable SPE for e500 CPU, I guess it makes sense to add it to the test.
Sep 19 2019
I made 8548 an alias in clang to e500, because e500 is recognized in llvm as a CPU, so gets us the feature list and, more importantly, the instruction scheduler.