- User Since
- Sep 8 2014, 1:26 PM (279 w, 6 d)
Tue, Jan 14
Mon, Jan 13
Fri, Jan 10
Fine with me. I just wanted -target powerpcspe-... to work with D72014.
Wed, Jan 8
Tue, Jan 7
Mon, Jan 6
Not happy, but won't hold it up.
Sun, Jan 5
Any more feedback on this?
Thu, Jan 2
@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.
Tue, Dec 31
Add triple unit test. I chose freebsd as the OS since that's my environment.
Mon, Dec 30
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.
Fri, Dec 27
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.
Sep 12 2019
Thanks for getting rid of SPE4RC, I really did not like it, but couldn't think of a better way. Glad you did.
Sep 5 2019
Not in 9.0, but I will try to push for it in 9.0.1.
Sep 3 2019
Thanks for pushing these through. They've been sitting in my private repo for too long now out of laziness.
Jul 29 2019
Should've marked it as need review earlier.
Jul 17 2019
Looks fine to me. Since it can be turned off, I don't see a problem if it causes issues.
Jul 16 2019
Jun 29 2019
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 revisited later.
Jun 28 2019
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?
The immediate offsets for the evldd/evstdd instructions are UInt8, not SInt8, but otherwise your change looks fine. Do you have additional tests for it? You had mentioned before about problematic relocations, is that still the case with your patch?
I removed the extra spill changes. Those can be done later if there actually is a problem. This didn't change the tests at all. I'm not quite sure how to write a test to demonstrate that "some but not all" registers are spilled in a given case, given that the register allocator can change at any time, so can't hard-code the registers that would get spilled, and I don't think I can even hard-code the number of registers that could get spilled.
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.
Jun 27 2019
Reduce the scope of the patch.
Address nemanjai's feedback. Move the EVX check into a separate callable
function. In the future it could possibly be used as a separate addressing
mode, but a naive approach didn't work, and this solves the problem at hand.
Jun 23 2019
Jun 16 2019
May 11 2019
Apr 29 2019
Apr 24 2019
Looks fine to me.
Apr 2 2019
Mar 28 2019
Mar 27 2019
Can you adjust the summary? This doesn't prevent floating point instructions completely, it only prevents floating point register constraints. The assertion that it prevents all floating point instructions is what I first took issue with, so please make it explicit that this is *only* regarding register constraints.
Reading through the diff again, closer, and checking the FreeBSD source, this is acceptable. FreeBSD only uses the base register, and hard-codes register numbers, so doesn't go through float register allocation.
Looks fine to me.
@kthomsen can you create a new revision just for that diff?
Mar 14 2019
I'd like to amend my previous comment: FreeBSD, and I'd guess Linux, too, explicitly builds the kernel with -msoft-float, in order to prevent FPU-consuming optimizations. However, we still need to be able to save and restore FPU context, which requires inline asm (or a completely asm file, not something we want). This change would prevent us being able to do that.
Mar 13 2019
I really don't like this. It should be possible to mix soft-float, Altivec, VSX, and SPE in the same file using inline asm. I put a check in for SPE codegen specifically to permit inline asm for other floating point models.