In D102342#3069943, @koakuma wrote:

In D102342#3060935, @dcederman wrote:

Hello!

It seems that this patch contains a rewrite of the remapRegsForLeafProc() function in addition to the new check described in the summary. Could you split it up into two patches, one with the rewrite and one with the added check?

I'm not sure about this. The way I see it is that the checks need to be done together with the rewrite, so it probably cannot be split like that (?)

Autogenerated assertions with update_llc_test_checks.py.

This looks good to me. Maybe change TailRelocSym to TailOpRelocSym or TailOperandRelocSym to make it more clear that it refers to the tail operand?

It seems that this patch contains a rewrite of the remapRegsForLeafProc() function in addition to the new check described in the summary. Could you split it up into two patches, one with the rewrite and one with the added check?

LGTM.

Moved DOTEXPR tests to end of file
Changed objdump offsets from dec to hex

I do not currently have commit access, but I have asked to so see if I can get it again and then I will push this patch.

Rebased on master. Changed unsigned into Register in a couple of places and changed "no-frame-pointer-elim" to "frame-pointer" in test.

It was some time since I worked with LLVM, but this looks good to me. It tries to fix a couple of things, so it would probably been better to divide it up into more than one patch and add some more tests, but this is fine. I compared the result with GAS for V8 and it matches.

I will try and review it next week.

Fixed warnings related to sign and unused parameters.

It seems that warnings are disabled by default for libunwind? Or I did something wrong when running cmake. Enabling warnings got me list of things I will have to fix before committing.

Added message to static_assert and moved the AArch64 check one step up.

Added assert to verify that DW_CFA_AARCH64_negate_ra_state and DW_CFA_GNU_window_save have the same value.

Added functionality to be able to differ between DW_CFA_AARCH64_negate_ra_state and DW_CFA_GNU_window_save.

For me native support is enough, so I removed the Sparc define from the !_LIBUNWIND_IS_NATIVE_ONLY section. That should make it compile with -DLIBUNWIND_ENABLE_CROSS_UNWINDING=ON.

On 64-bit Sparc we can allow tail call optimizations on functions that returns a struct even though the caller does not (and vice versa).

Do not tail call opt if either the callee or caller returns a struct and the other does not.

Removed empty lines and moved the trace message to before the operation.

Thanks for the review!

I have been using this Sparc version of libunwind when running the libc++ testsuite and have not had any problems related to libunwind. Does anyone have any additional comments or is it ok to commit?

This update does not use __builtin_extract_return_addr in _Unwind_GetIP. Instead it modifies the return address in stepWithDwarf, similar to aarch64. It also checks if the instruction at the return address is an unimp instruction. If it is, that means that the function is returning a struct and that we should skip one extra instruction.

Only include DW_CFA_GNU_window_save support when compiling for Sparc.

Added a check to make sure that the integer mask is between 0 and 127 when parsing. The immediate is now printed as an integer when outside the valid range. Also fixed a disassembly issue and added some more tests.

Any other comments @jyknight , or does it look ok?

Removed the UseRegG* features and enabled the ReserveRegG6, ReserveRegG7, and ReserveRegG5 (on 32-bit) features by default.

See https://reviews.llvm.org/D51487 instead.

Rebased and changed objdump output check from Unknown to symbol name (Local/Extern).

In D36463#1221777, @fedor.sergeev wrote:

Ok for me. Are you willing to take the ownership of this?

The assert does not trigger after https://reviews.llvm.org/rL334383. Is it ok to apply this patch? Just need to change the objdump output from Unknown to the symbol name.

The partial write psr instruction is now part of binutils (https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=df28970).

Yes, the SPARC V8e manual did not provide a mnemonic for it so it is invented here. I have submitted a similar patch to binutils so I will hold this patch until the other one gets approved, in case someone wants me to change the name.

I'm fixing them for the failures and to learn about LLVM. I have no use case and I'm perfectly fine with and in favor of removing the support. Can I just revert the patch that introduced the support, or should I submit a removal patch for review?

When I was testing the code I saw that it sometimes generated two versions of the same constant, one with the single SETHI with the complement form, and one with the standard SETHI/ORri pair. So I added the hasOneUse() part to prevent the case when a constant is encoded using three instructions instead of just one or two.

In D48638#1213699, @jyknight wrote:

LGTM.

It's possibly only theoretical at the moment, since I don't know if anything both supports this and is little endian, but --
On a little endian system, will the registers be swapped; that is, would ASR22 hold the 32 LSB instead of ASR23?

Reserving registers ASR1-ASR31

Added attributes to prevent %g2-%g5 from being used as scratch register or to allow the use of %g5-%g7 as scratch registers. %g5 is reserved by default for 32-bit but not for 64-bit.

Moved code into a PerformDAGCombine function.