Superseded by D151711 .

Thanks for dropping the lambda, it's now cleaner than my original patch as well. I know @sfertile asked me for a simpler test case, but I don't think it's really feasible.

Looks good to me, thanks!

Adding a couple explicit reviewers.

I love the goal of this, but it looks incomplete. I only noted on one, but it's probably the same for the other tests, that the change will end up clobbering the top half of registers without saving and restoring them.

This looks just like D78669, which I've admittedly been very lazy on.

https://github.com/llvm/llvm-project/commit/f43b2285815961da057af1a772bc31d0152d286b

@nemanjai Any further comment on this, or is it good to go?

Add test case into fma-assoc test. This appears to be the simplest test to validate the change.

The spe.ll respin looks fine to me.

Looks fine to me. @nemanjai this one is more Musl, less SPE :)

We use compiler-rt in FreeBSD for all platforms, including powerpcspe.

To be honest, I don't know the correct intrinsic. SPE is a strange beast, and effectively requires support from the operating system in order to behave comparable to a traditional FPU. What you have looks good to me.

Turns out GCC generates 1200-range register values. This still needs work to avoid the error, but it means giving libunwind special knowledge of the SPE register set.

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.

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.

Ping?

Ping?

Address feedback from @shchenz. Much better!

@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.

In D77558#1966207, @shchenz wrote:

This happens too late for the CTR usability test to veto using the CTR in a loop, and results in an assert "Invalid PPC CTR loop!".

This means after not hoisting fmul makes a case have "Invalid PPC CTR loop!" assertion? This is a little surprised for me. Hoist or not hoist fmul should not impact CTR register.
Looking forward to your case.

In D77433#1961097, @compnerd wrote:

Im surprised that no tests need changing here. Could you please add a clang/llvm test to validate that the generated information matches what libunwind will start looking for?

Code looks fine, and others have tested it. Good to see a reversion of the ABI to expected for GCC compatibility on the BSDs.

Can you fix the title of this? "Fix PR45297" is not very descriptive unless someone also reviews the PR.

Update tests. Updated other tests in the same file that looked suspect as well.

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.

Fine with me. I just wanted -target powerpcspe-... to work with D72014.

Not happy, but won't hold it up.

Any more feedback on this?

@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.

Add triple unit test. I chose freebsd as the OS since that's my environment.

Reuse the SPE feature test preprocessor test check instead of duplicating it.
powerpcspe-* is exactly equivalent to the -mspe command line argument.

In D69483#1799302, @jsji wrote:

Looks like to me that Vector Compare in SPE also has different CR bit semantics .
So this patch will only handle floating point part for SPE? If so, maybe we should make it explicit in title.

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.

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.

@kthomsen do you have any tests we can massage for this?

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
for review.

Update diff. It would help if I actually compiled when rebasing...

Rebase

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.

Add tests, taken from the C test in comments in D54583.

In D70570#1778165, @sfertile wrote:

@MaskRay , @jhibbits Thank you for providing the correct ABI doc, and for the answers. I'll start going through it.

In D70570#1777864, @jhibbits wrote:

• It is an error to not use a PLTREL relocation for a call that needs to be indirected through the PLT. The linker will complain to recompile with -fPIC.

Means you can't link static execs against shared objects anymore. Is this the intent? Or am I missing something obvious?

@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.

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.

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.

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.