Re: [PATCH] i386: Fix regression after refactoring legitimize_pe_coff_symbol, ix86_GOT_alias_set and PE_COFF_LEGITIMIZE_EXTERN_DECL

[Uros Bizjak]

On Thu, Jun 27, 2024 at 12:50 PM Evgeny Karpov
<evgeny.kar...@microsoft.com> wrote:
>
> Thursday, June 27, 2024 10:39 AM
> Uros Bizjak <ubiz...@gmail.com> wrote:
>
> > > diff --git a/gcc/config/i386/i386-expand.cc 
> > > b/gcc/config/i386/i386-expand.cc
> > > index 5dfa7d49f58..20adb42e17b 100644
> > > --- a/gcc/config/i386/i386-expand.cc
> > > +++ b/gcc/config/i386/i386-expand.cc
> > > @@ -414,6 +414,10 @@ ix86_expand_move (machine_mode mode, rtx
> > operands[])
> > >   {
> > >  #if TARGET_PECOFF
> > >    tmp = legitimize_pe_coff_symbol (op1, addend != NULL_RTX);
> > > +#else
> > > +    tmp = NULL_RTX;
> > > +#endif
> > > +
> > >    if (tmp)
> > >      {
> > >        op1 = tmp;
> > > @@ -425,7 +429,6 @@ ix86_expand_move (machine_mode mode, rtx
> > operands[])
> > >        op1 = operands[1];
> > >        break;
> > >      }
> > > -#endif
> > >   }
> > >
> > >        if (addend)
> >
> > tmp can only be set by legitimize_pe_coff_symbol, so !TARGET_PECOFF
> > will always get to the "else" part. Do this change simply by moving
> > #endif, like the below:
> >
> > --cut here--
> > iff --git a/gcc/config/i386/i386-expand.cc b/gcc/config/i386/i386-expand.cc
> > index 5dfa7d49f58..407db6c215b 100644
> > --- a/gcc/config/i386/i386-expand.cc
> > +++ b/gcc/config/i386/i386-expand.cc
> > @@ -421,11 +421,11 @@ ix86_expand_move (machine_mode mode, rtx
> > operands[])
> >                break;
> >            }
> >          else
> > +#endif
> >            {
> >              op1 = operands[1];
> >              break;
> >            }
> > -#endif
> >        }
> >
> >       if (addend)
> > --cut here--
> >
>
> I would prefer readability in the original version if there are no objections.

The proposed form is how existing TARGET_MACHO handles similar issue.
Please see e.g. i386.cc, around line 6216 and elsewhere:

#if TARGET_MACHO
  if (TARGET_MACHO)
    {
      switch_to_section (darwin_sections[picbase_thunk_section]);
      fputs ("\t.weak_definition\t", asm_out_file);
      assemble_name (asm_out_file, name);
      fputs ("\n\t.private_extern\t", asm_out_file);
      assemble_name (asm_out_file, name);
      putc ('\n', asm_out_file);
      ASM_OUTPUT_LABEL (asm_out_file, name);
      DECL_WEAK (decl) = 1;
    }
  else
#endif
    if (USE_HIDDEN_LINKONCE)
...

So, there is no problem having #endif just after else.

Anyway, it's your call, this is not a hill I'm willing to die on. ;)

Thanks,
Uros.