Re: [PATCH ver4] rs6000, Add return value to __builtin_set_fpscr_rn
Hi Carl, on 2023/7/12 02:06, Carl Love wrote: > GCC maintainers: > > Ver 4, Removed extra space in subject line. Added comment to commit > log comments about new __SET_FPSCR_RN_RETURNS_FPSCR__ define. Changed > Added to Add and Renamed to Rename in ChangeLog. Updated define_expand > "rs6000_set_fpscr_rn" per Peter's comments to use new temporary > register for output value. Also, comments from Kewen about moving rtx > tmp_di1 close to use. Renamed tmp_di2 as orig_df_in_di. Additionally, > changed the name of tmp_di3 to tmp_di2 so the numbering is > sequential. Moved the new rtx tmp_di2 = gen_reg_rtx (DImode); right > before its use to be consistent with previous move request. Fixed tabs > in comment. Remove -std=c99 from test_fpscr_rn_builtin_1.c. Cleaned up > comment and removed abort from test_fpscr_rn_builtin_2.c. > > Fixed a couple of additional issues with the ChangeLog per feedback > from git gcc-verify. > > Retested updated patch on Power 8, 9 and 10 to verify changes. > > Ver 3, Renamed the patch per comments on ver 2. Previous subject line > was " [PATCH ver 2] rs6000, __builtin_set_fpscr_rn add retrun value". > Fixed spelling mistakes and formatting. Updated define_expand > "rs6000_set_fpscr_rn to have the rs6000_get_fpscr_fields and > rs6000_update_fpscr_rn_field define expands inlined. Optimized the > code and fixed use of temporary register values. Updated the test file > dg-do run arguments and dg-options. Removed the check for > __SET_FPSCR_RN_RETURNS_FPSCR__. Removed additional references to the > overloaded built-in with double argument. Fixed up the documentation > file. Updated patch retested on Power 8 BE/LE, Power 9 BE/LE and Power > 10 LE. > > Ver 2, Went back thru the requirements and emails. Not sure where I > came up with the requirement for an overloaded version with double > argument. Removed the overloaded version with the double argument. > Added the macro to announce if the __builtin_set_fpscr_rn returns a > void or a double with the FPSCR bits. Updated the documentation file. > Retested on Power 8 BE/LE, Power 9 BE/LE, Power 10 LE. Redid the test > file. Per request, the original test file functionality was not > changed. Just changed the name from test_fpscr_rn_builtin.c to > test_fpscr_rn_builtin_1.c. Put new tests for the return values into a > new test file, test_fpscr_rn_builtin_2.c. > > The GLibC team requested a builtin to replace the mffscrn and > mffscrniinline asm instructions in the GLibC code. Previously there > was discussion on adding builtins for the mffscrn instructions. > > https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620261.html > > In the end, it was felt that it would be to extend the existing > __builtin_set_fpscr_rn builtin to return a double instead of a void > type. The desire is that we could have the functionality of the > mffscrn and mffscrni instructions on older ISAs. The two instructions > were initially added in ISA 3.0. The __builtin_set_fpscr_rn has the > needed functionality to set the RN field using the mffscrn and mffscrni > instructions if ISA 3.0 is supported or fall back to using logical > instructions to mask and set the bits for earlier ISAs. The > instructions return the current value of the FPSCR fields DRN, VE, OE, > UE, ZE, XE, NI, RN bit positions then update the RN bit positions with > the new RN value provided. > > The current __builtin_set_fpscr_rn builtin has a return type of void. > So, changing the return type to double and returning the FPSCR fields > DRN, VE, OE, UE, ZE, XE, NI, RN bit positions would then give the > functionally equivalent of the mffscrn and mffscrni instructions. Any > current uses of the builtin would just ignore the return value yet any > new uses could use the return value. So the requirement is for the > change to the __builtin_set_fpscr_rn builtin to be backwardly > compatible and work for all ISAs. > > The following patch changes the return type of the > __builtin_set_fpscr_rn builtin from void to double. The return value > is the current value of the various FPSCR fields DRN, VE, OE, UE, ZE, > XE, NI, RN bit positions when the builtin is called. The builtin then > updated the RN field with the new value provided as an argument to the > builtin. The patch adds new testcases to test_fpscr_rn_builtin.c to > check that the builtin returns the current value of the FPSCR fields > and then updates the RN field. > > The GLibC team has reviewed the patch to make sure it met their needs > as a drop in replacement for the inline asm mffscr and mffscrni > statements in the GLibC code. T > > The patch has been tested on Power 8 LE/BE, Power 9 LE/BE and Power 10 > LE. > > Please let me know if the patch is acceptable for mainline. Thanks. > >Carl > > - > rs6000, Add return value to __builtin_set_fpscr_rn > > Change the return value from void to double for
[PATCH ver4] rs6000, Add return value to __builtin_set_fpscr_rn
GCC maintainers: Ver 4, Removed extra space in subject line. Added comment to commit log comments about new __SET_FPSCR_RN_RETURNS_FPSCR__ define. Changed Added to Add and Renamed to Rename in ChangeLog. Updated define_expand "rs6000_set_fpscr_rn" per Peter's comments to use new temporary register for output value. Also, comments from Kewen about moving rtx tmp_di1 close to use. Renamed tmp_di2 as orig_df_in_di. Additionally, changed the name of tmp_di3 to tmp_di2 so the numbering is sequential. Moved the new rtx tmp_di2 = gen_reg_rtx (DImode); right before its use to be consistent with previous move request. Fixed tabs in comment. Remove -std=c99 from test_fpscr_rn_builtin_1.c. Cleaned up comment and removed abort from test_fpscr_rn_builtin_2.c. Fixed a couple of additional issues with the ChangeLog per feedback from git gcc-verify. Retested updated patch on Power 8, 9 and 10 to verify changes. Ver 3, Renamed the patch per comments on ver 2. Previous subject line was " [PATCH ver 2] rs6000, __builtin_set_fpscr_rn add retrun value". Fixed spelling mistakes and formatting. Updated define_expand "rs6000_set_fpscr_rn to have the rs6000_get_fpscr_fields and rs6000_update_fpscr_rn_field define expands inlined. Optimized the code and fixed use of temporary register values. Updated the test file dg-do run arguments and dg-options. Removed the check for __SET_FPSCR_RN_RETURNS_FPSCR__. Removed additional references to the overloaded built-in with double argument. Fixed up the documentation file. Updated patch retested on Power 8 BE/LE, Power 9 BE/LE and Power 10 LE. Ver 2, Went back thru the requirements and emails. Not sure where I came up with the requirement for an overloaded version with double argument. Removed the overloaded version with the double argument. Added the macro to announce if the __builtin_set_fpscr_rn returns a void or a double with the FPSCR bits. Updated the documentation file. Retested on Power 8 BE/LE, Power 9 BE/LE, Power 10 LE. Redid the test file. Per request, the original test file functionality was not changed. Just changed the name from test_fpscr_rn_builtin.c to test_fpscr_rn_builtin_1.c. Put new tests for the return values into a new test file, test_fpscr_rn_builtin_2.c. The GLibC team requested a builtin to replace the mffscrn and mffscrniinline asm instructions in the GLibC code. Previously there was discussion on adding builtins for the mffscrn instructions. https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620261.html In the end, it was felt that it would be to extend the existing __builtin_set_fpscr_rn builtin to return a double instead of a void type. The desire is that we could have the functionality of the mffscrn and mffscrni instructions on older ISAs. The two instructions were initially added in ISA 3.0. The __builtin_set_fpscr_rn has the needed functionality to set the RN field using the mffscrn and mffscrni instructions if ISA 3.0 is supported or fall back to using logical instructions to mask and set the bits for earlier ISAs. The instructions return the current value of the FPSCR fields DRN, VE, OE, UE, ZE, XE, NI, RN bit positions then update the RN bit positions with the new RN value provided. The current __builtin_set_fpscr_rn builtin has a return type of void. So, changing the return type to double and returning the FPSCR fields DRN, VE, OE, UE, ZE, XE, NI, RN bit positions would then give the functionally equivalent of the mffscrn and mffscrni instructions. Any current uses of the builtin would just ignore the return value yet any new uses could use the return value. So the requirement is for the change to the __builtin_set_fpscr_rn builtin to be backwardly compatible and work for all ISAs. The following patch changes the return type of the __builtin_set_fpscr_rn builtin from void to double. The return value is the current value of the various FPSCR fields DRN, VE, OE, UE, ZE, XE, NI, RN bit positions when the builtin is called. The builtin then updated the RN field with the new value provided as an argument to the builtin. The patch adds new testcases to test_fpscr_rn_builtin.c to check that the builtin returns the current value of the FPSCR fields and then updates the RN field. The GLibC team has reviewed the patch to make sure it met their needs as a drop in replacement for the inline asm mffscr and mffscrni statements in the GLibC code. T The patch has been tested on Power 8 LE/BE, Power 9 LE/BE and Power 10 LE. Please let me know if the patch is acceptable for mainline. Thanks. Carl - rs6000, Add return value to __builtin_set_fpscr_rn Change the return value from void to double for __builtin_set_fpscr_rn. The return value consists of the FPSCR fields DRN, VE, OE, UE, ZE, XE, NI, RN bit positions. A new test file, test powerpc/test_fpscr_rn_builtin_2.c, is added to test the new return value for the built-in. The value
