Re: syncing the GCC vax port

[coypu%sdf.org@localhost]

Sorry for the delay...
Updated diff: http://coypu.sdf.org/vax-gcc10.diff

On Mon, Apr 29, 2019 at 02:08:32PM -0600, Jeff Law wrote:
> On 3/30/19 3:03 AM, coypu%sdf.org@localhost wrote:
> > hi folks,
> > 
> > i was interesting in tackling some problems gcc netbsd/vax has.
> > it has some ICEs which are in reload phase. searching around, the answer
> > to that is "switch to LRA first". Now, I don't quite know what that is
> > yet, but I know I need to try to do it.
> > 
> > As an initial step, I need to sync the source code.
> > netbsd/vax has some outstanding work on GCC.
> > 
> > I've done this, and I can run programs built by this compiler:
> > http://coypu.sdf.org/gcc-9-vax.diff
> > 
> > (My tree has more detail on the changes done:
> > https://github.com/gcc-mirror/gcc/compare/master...coypoop:vax )
> > 
> > Matt Thomas (the GCC VAX maintainer) is the author of most of these
> > changes, I suspect he will not be very responsive by email.
> > Not being the author, I might not be able to answer all the questions,
> > but I can try my best.
> > 
> > How do I get this across? comments? straight to gcc-patches? :-)
> > 
> > I know Jeff Law did not like the change to builtins.md as being wrong. I
> > can omit them, I forgot about it until typing this email :)
> > 
> > caveat: had an ICE during reload in the build process, I hid it
> > under the rug with -O0.
> I don't recall what I objected to :-)  From looking at the changes I'd
> be concerned about the changes from memory_operand to
> volatile_mem_operand.  Without knowing more about the problem you're
> trying to solve it's hard to ACK something like this.

Removed from the diff. Unfortunately this introduces an ICE during the
build of GCC (but that will be the second kind)

> The new "condjump" expander seems a bit under-specified.  Is there some
> reason why you needed that expander and couldn't just add a name to a
> existing define_insn?

I didn't find an answer for this yet. What I got so far:

Apparently vax did not include builtins.md before, and so an adjustment
that should have happened when the cond-optab branch was merged didn't
happen.

> A nit.  In that expander you have "gen_rtx_NE(VOIDmode ..."  There
> should be a space between the NE and the open parenthesis.  That kind of
> nit is repeated several times.

Fixed.

> The changes in constraints.md do not seem to change functionality at
> all, just reordering the oprerands.  However our preferred style is
> what's in there right now.

Undid that.

> ival >= 0  is the right way   0 <= ival is the wrong way.

Fixed.

> I noticed the patch turns off flag_dwarf2_cfi_asm.  Is there a reason
> for that?  But yet you create DEF_CFA notes in the prologue.  Is it the
> case that the CFI bits just aren't ready for consumption yet?  If not,
> then it may be easier to just not include those changes right now.

This is due to a binutils issue. It looks similar to
https://github.com/riscv/riscv-binutils-gdb/issues/76
I have yet to find a fix for it.

> I can't really comment on the changes to how addreses are handled in
> print_operand_address.  I'd just have to assume they're correct.
> 
> I don't know if we generally allow debug_rtx calls like are added.
> Usually we gcc_assert or gcc_unreachable.

OK. I removed those and added a single gcc_unreachable.

> mkrtx needs a function comment and looks like its got some formatting
> problems (spaces vs tabs issues and missing space between function name
> an the open paren for arguments).  Similarly for vax_output_movmemsi and
> legitimate_pic_operand_p, vax_decomposed_dimode_operand_p,

Added comments.

> You've got undocumented #ifdefs in legitimate_pic_operand_p.

elf.h contains:

/* Don't allow *foo which foo is non-local */
#define NO_EXTERNAL_INDIRECT_ADDRESS

Is this sufficient? thanks.

> You've got incorrect operand ordering in the adjacent_operands_p changes.

Fixed.

> The netbsd specific changes to the zero_extract patterns looks strange.
>  Why why not just have the right operand.  And why change it in the
> first place?

Refers to same as first comment: I removed these changes. Back to the
drawing board with that crash.

> Those peepholes look strange.  Why is the first insn not just removed as
> dead?

I don't understand this comment (sorry). Can you clarify it?
I removed one peephole that was (0 &&...), if that helps.

> And if these changes were done by Matt Thomas, does he have a copyright
> assignment on file?  If not, then we can't really use them.  These are
> large enough that they'd require an assignment.

Yes, Matt Thomas has a copyright assignment on file.