Jeff Law <jeffreya...@gmail.com> writes:
> On 6/17/24 3:53 AM, Richard Sandiford wrote:
>> This patch makes target-independent code use force_subreg instead
>> of simplify_gen_subreg in some places. The criteria were:
>>
>> (1) The code is obviously specific to expand (where new pseudos
>> can be created), or at least would be invalid to call when
>> !can_create_pseudo_p () and temporaries are needed.
>>
>> (2) The value is obviously an rvalue rather than an lvalue.
>>
>> (3) The offset wasn't a simple lowpart or highpart calculation;
>> a later patch will deal with those.
>>
>> Doing this should reduce the likelihood of bugs like PR115464
>> occuring in other situations.
>>
>> gcc/
>> * expmed.cc (store_bit_field_using_insv): Use force_subreg
>> instead of simplify_gen_subreg.
>> (store_bit_field_1): Likewise.
>> (extract_bit_field_as_subreg): Likewise.
>> (extract_integral_bit_field): Likewise.
>> (emit_store_flag_1): Likewise.
>> * expr.cc (convert_move): Likewise.
>> (convert_modes): Likewise.
>> (emit_group_load_1): Likewise.
>> (emit_group_store): Likewise.
>> (expand_assignment): Likewise.
> [ ... ]
>
> So this has triggered a failure on ft32-elf with this testcase
> (simplified from the testsuite):
>
> typedef _Bool bool;
> const bool false = 0;
> const bool true = 1;
>
> struct RenderBox
> {
> bool m_positioned : 1;
> };
>
> typedef struct RenderBox RenderBox;
>
>
> void RenderBox_setStyle(RenderBox *thisin)
> {
> RenderBox *this = thisin;
> bool ltrue = true;
> this->m_positioned = ltrue;
>
> }
>
>
>
> Before this change we generated this:
>
>> (insn 13 12 14 (set (reg:QI 47)
>> (mem/c:QI (plus:SI (reg/f:SI 37 virtual-stack-vars)
>> (const_int -5 [0xfffffffffffffffb])) [1 ltrue+0 S1 A8]))
>> "j.c":17:22 -1
>> (nil))
>>
>> (insn 14 13 15 (parallel [
>> (set (zero_extract:SI (subreg:SI (reg:QI 46) 0)
>> (const_int 1 [0x1])
>> (const_int 0 [0]))
>> (subreg:SI (reg:QI 47) 0))
>> (clobber (scratch:SI))
>> ]) "j.c":17:22 -1
>> (nil))
>
>
> Afterwards we generate:
>
>> (insn 13 12 14 2 (parallel [
>> (set (zero_extract:SI (subreg:SI (reg:QI 46) 0)
>> (const_int 1 [0x1])
>> (const_int 0 [0]))
>> (subreg:SI (mem/c:QI (plus:SI (reg/f:SI 37
>> virtual-stack-vars)
>> (const_int -5 [0xfffffffffffffffb])) [1 ltrue+0
>> S1 A8]) 0))
>> (clobber (scratch:SI))
>> ]) "j.c":17:22 -1
>> (nil))
>
> Note the (subreg (mem (...)). Probably not desirable in general, but
> also note the virtual-stack-vars in the memory address. The code to
> instantiate virtual registers doesn't handle (subreg (mem)), so we never
> convert that to an FP based address and we eventually fault.
>
> Should be visible with ft32-elf cross compiler. No options needed.
Bah. Thanks for the report.
I agree of course with the follow-on discussion that we should get
rid of (subreg (mem)). But this was supposed to be a conservative
patch. I've therefore reverted the offending part of the commit,
as below. (Tested on aarch64-linux-gnu.)
Richard
One of the changes in g:d4047da6a070175aae7121c739d1cad6b08ff4b2
caused a regression in ft32-elf; see:
https://gcc.gnu.org/pipermail/gcc-patches/2024-June/655418.html
for details. This change was different from the others in that the
original call was to simplify_subreg rather than simplify_lowpart_subreg.
The old code would therefore go on to do the force_reg for more cases
than the new code would.
gcc/
* expmed.cc (store_bit_field_using_insv): Revert earlier change
to use force_subreg instead of simplify_gen_subreg.
---
gcc/expmed.cc | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/gcc/expmed.cc b/gcc/expmed.cc
index 3b9475f5aa0..8bbbc94a98c 100644
--- a/gcc/expmed.cc
+++ b/gcc/expmed.cc
@@ -695,7 +695,13 @@ store_bit_field_using_insv (const extraction_insn *insv,
rtx op0,
if we must narrow it, be sure we do it correctly. */
if (GET_MODE_SIZE (value_mode) < GET_MODE_SIZE (op_mode))
- tmp = force_subreg (op_mode, value1, value_mode, 0);
+ {
+ tmp = simplify_subreg (op_mode, value1, value_mode, 0);
+ if (! tmp)
+ tmp = simplify_gen_subreg (op_mode,
+ force_reg (value_mode, value1),
+ value_mode, 0);
+ }
else
{
if (targetm.mode_rep_extended (op_mode, value_mode) != UNKNOWN)
--
2.25.1
