syncing the GCC vax port
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.
Re: syncing the GCC vax port
On Sun, Mar 31, 2019 at 01:25:50PM -0400, Paul Koning wrote: > > > > On Mar 30, 2019, at 5:03 AM, co...@sdf.org 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. > > That's not quite the whole story. > > The answer is (1) switch from CC0 to CCmode condition code handling, which > enables (2) switch from Reload to LRA. > > (1) requires actual work, not terribly hard but not entirely trivial. (2) > may take as little as switching the "use LRA" flag to "yes". > > I did (1) as well as a tentative (2) for pdp11 last year. It was reasonably > straightforward thanks to a pile of help from Eric Botcazou and his gcc wiki > articles on the subject. You might find the pdp11 deltas for CCmode helpful > as a source of ideas, since the two machines have a fair amount in common as > far as condition codes goes. At least for the integer ops (pdp11 has > separate floating point conditions, vax doesn't). > > paul > Hi paul! I have been reading on this, so now I have a draft that compiles the world's simplest C code (and nothing more, it will crash), but using CCmode (I think). I am being inspired by your port (which is a good thing since I know I can ask questions about it :)) https://github.com/coypoop/gcc/commit/df135c019de33950c9997fdea3ce07c5c920384d (I know that it's wrong!)
Re: Deprecate -traditional-cpp
Just chiming in. My buddy wrote a traditional C pre-processor for use with Imake (and, presumably, other old things). https://www.netbsd.org/~dholland/tradcpp/ (It's standalone).
Re: syncing the GCC vax port
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, co...@sdf.org 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.
Re: syncing the GCC vax port, atomic issue
On Fri, Sep 20, 2019 at 11:15:32AM +, co...@sdf.org wrote: > Removed from the diff. Unfortunately this introduces an ICE during the > build of GCC... I took another look at the VAX atomic pattern issue. (http://gnats.netbsd.org/53039) It is a compiler crash to do with the added atomic builtins. The original suggestion was to introduce a reversed pattern, with label / pc swapped. It did not work, unfortunately. The crash backtrace is (gcc-trunk line numbers): during RTL pass: expand /home/fly/gcc/libatomic/flag.c: In function 'atomic_flag_test_and_set': /home/fly/gcc/libatomic/flag.c:36:1: internal compiler error: in patch_jump_insn, at cfgrtl.c:1291 36 | } | ^ 0x718c22 patch_jump_insn /home/fly/gcc/gcc/cfgrtl.c:1290 0x718d2f redirect_branch_edge /home/fly/gcc/gcc/cfgrtl.c:1317 0x71b8c2 rtl_redirect_edge_and_branch /home/fly/gcc/gcc/cfgrtl.c:1450 0x703909 redirect_edge_and_branch(edge_def*, basic_block_def*) /home/fly/gcc/gcc/cfghooks.c:373 0x119ec6e try_forward_edges /home/fly/gcc/gcc/cfgcleanup.c:558 0x11a26ca try_optimize_cfg /home/fly/gcc/gcc/cfgcleanup.c:2961 0x11a26ca cleanup_cfg(int) /home/fly/gcc/gcc/cfgcleanup.c:3175 0x700a41 execute /home/fly/gcc/gcc/cfgexpand.c:6683 This seems to be where GCC has some logic specific to optimizing jumps. Since this isn't a normal jump possible to eliminate by being clever, but rather our only way of doing atomics, my gut feeling is that I should avoid this jump optimizing code entirely. Not telling GCC it's a jump worth optimizing seems to avoid the crash, i.e.: - emit_jump_insn (gen_jbbssi (operands[1], const0_rtx, label, operands[1])); + emit_insn (gen_jbbssi (operands[1], const0_rtx, label, operands[1])); + LABEL_NUSES (label)++; GCC builds, but when building netbsd I get an undefined reference in libstdc++: libstdc++.so: undefined reference to `.L72' libstdc++.so: undefined reference to `.L75' I wonder whether this is a right approach with a slightly off method.
Re: syncing the GCC vax port, atomic issue
On Fri, Sep 20, 2019 at 03:45:46PM -0600, Jeff Law wrote: > Conditional branching patterns must support the label_ref and pc > operands in either position. Everything else I've seen on this thread > is just working around that broken aspect of the builtins.md file. > > > (define_insn "jbbssiqi" > [(parallel > [(set (pc) > (if_then_else > (ne (zero_extract:SI (match_operand:QI 0 "memory_operand" "g") >(const_int 1) >(match_operand:SI 1 "general_operand" "nrm")) > (const_int 0)) > (label_ref (match_operand 2 "" "")) > (pc))) > (set (zero_extract:SI (match_operand:QI 3 "memory_operand" "+0") > (const_int 1) > (match_dup 1)) > (const_int 1))])] > "" > "jbssi %1,%0,%l2") > > Note the position of the (label_ref ...) and (pc) operand. You have to > have a pattern where they are reversed as well. > > As an example look at the branch and branch_reversed patterns in vax.md > > jeff Hi Jeff, Thanks for the advice. Introducing the reversed jbb* patterns doesn't seem to help with the original issue. It crashes building libatomic. I've attempted the following diff, as suggested by Maciej W. Rozycki. (This isn't failing in the latest GCC because it doesn't include vax/builtins.md at all) > Ouch, there are no reversed interlocked branch instructions in the VAX > ISA, so these would have to branch around a jump. ... https://gcc.gnu.org/ml/gcc/2018-04/msg00073.html Is it the intended diff? did I get something wrong? diff --git a/gcc/config/vax/builtins.md b/gcc/config/vax/builtins.md index 2261467..db8ddc40 100644 --- a/gcc/config/vax/builtins.md +++ b/gcc/config/vax/builtins.md @@ -90,6 +90,24 @@ "" "jbssi %1,%0,%l2") +(define_insn "jbbssiqi_reversed" + [(parallel +[(set (pc) + (if_then_else + (ne (zero_extract:SI (match_operand:QI 0 "memory_operand" "g") +(const_int 1) +(match_operand:SI 1 "general_operand" "nrm")) + (const_int 0)) + (pc) + (label_ref (match_operand 2 "" "" + (set (zero_extract:SI (match_operand:QI 3 "memory_operand" "+0") + (const_int 1) + (match_dup 1)) + (const_int 1))])] + "" + "jbssi %1,%0,0f\;jbr %l2\;0:") + + (define_insn "jbbssihi" [(parallel [(set (pc) @@ -107,6 +125,24 @@ "" "jbssi %1,%0,%l2") +(define_insn "jbbssihi_reversed" + [(parallel +[(set (pc) + (if_then_else +(ne (zero_extract:SI (match_operand:HI 0 "memory_operand" "Q") + (const_int 1) + (match_operand:SI 1 "general_operand" "nrm")) +(const_int 0)) +(pc) +(label_ref (match_operand 2 "" "" + (set (zero_extract:SI (match_operand:HI 3 "memory_operand" "+0") + (const_int 1) + (match_dup 1)) + (const_int 1))])] + "" + "jbssi %1,%0,0f\;jbr %l2\;0:") + + (define_insn "jbbssisi" [(parallel [(set (pc) @@ -125,6 +161,25 @@ "jbssi %1,%0,%l2") +(define_insn "*jbbssisi_reversed" + [(parallel +[(set (pc) + (if_then_else + (ne (zero_extract:SI (match_operand:SI 0 "memory_operand" "Q") +(const_int 1) +(match_operand:SI 1 "general_operand" "nrm")) + (const_int 0)) + (pc) + (label_ref (match_operand 2 "" "" + (set (zero_extract:SI (match_operand:SI 3 "memory_operand" "+0") + (const_int 1) + (match_dup 1)) + (const_int 1))])] + "" + "jbssi %1,%0,0f\; jbr %l2\;0:") + + + (define_expand "sync_lock_release" [(set (match_operand:VAXint 0 "memory_operand" "+m") (unspec:VAXint [(match_operand:VAXint 1 "const_int_operand" "n") @@ -162,6 +217,23 @@ "" "jbcci %1,%0,%l2") +(define_insn "jbbcciqi_reversed" + [(parallel +[(set (pc) + (if_then_else + (eq (zero_extract:SI (match_operand:QI 0 "memory_operand" "g") +(const_int 1) +(match_operand:SI 1 "general_operand" "nrm")) + (const_int 0)) + (pc) + (label_ref (match_operand 2 "" "" + (set (zero_extract:SI (match_operand:QI 3 "memory_operand" "+0") + (const_int 1) + (match_dup 1)) + (const_int 0))])] + "" + "jbcci %1,%0,0f\; jbr %l2\;0:") + (define_insn "jbbccihi" [(parallel [(set (pc) @@ -179,6 +251,24 @@ "" "jbcci %1,%0,%l2") + +(define_insn "jbbccihi_reversed" + [(parallel +[(set (pc) + (if_then_else + (eq (zero_extract:SI (match_operand:HI 0 "memory_operand" "Q")
Re: syncing the GCC vax port, atomic issue
On Fri, Sep 20, 2019 at 10:07:59PM +, co...@sdf.org wrote: > Introducing the reversed jbb* patterns doesn't seem to help with the > original issue. It crashes building libatomic. My loose understanding of what is going on: - GCC emits this atomic in expand. - When cleaning up, it looks for optimizations. - It decides this is a branch to another branch situation, so maybe can be improved. - This fails to output an instruction for unrelated reasons. - Hit an assert. I don't think that we should be trying to combine regular branch + atomic branch in very generic code. My guess is that, if it didn't crash now, it might emit a different kind of branch which loses the atomic qualities, and result in wrong code. I tried to single-step GCC, and it might be trying entirely different instruction patterns. I'm not sure whether I should put a lot of trust in the line numbers shown from .md files, but it's trying nonlocal_goto in vax.md. In any case, nothing from builtins.md.
Upstreaming very old changes
Hi, GCC! I believe netbsd is the primary user of the vax target. its status is: good: netbsd uses gcc 5.4.0, and cross compiles its userland+kernel with this. it runs and is also able to natively build useful programs like perl. bad: -O0 in places, text relocations. obvious signs of more bugs not yet investigated. however, this is with a large diff to gcc. the author of most of the changes is Matt Thomas, who is also the GCC vax port maintainer. the biggest change is probably shared library support (... from 1998). I'm not sure why he hasn't committed his work upstream, but it would be nice to bridge the gap, to make it easier for anyone (possibly myself, in some future time after education) to adapt the code to non-deprecated GCC APIs. legal - I don't think this is a real issue - most people involved have already signed FSF paperwork, and there's commit history, so I can explicitly ask all the people involved to state they are OK with it. technical - I am not a compiler expert, and this is a large unexplained diff. even originally, the commit messages weren't very verbose. It also adds 20 years of effectively "merge local diff to head". however, the end result does work well enough to boot, run, etc. if I go down this road, how can I begin bridging this gap? Thanks.
Internal compiler error building libstdc++ for vax
Hi folks,
netbsd's copy of GCC differs enough that it fails elsewhere with
gcc-trunk, but the problematic code is upstream.
updating netbsd to gcc 6.4.0, I get an internal compiler error building
libstdc++. (Long version: http://gnats.netbsd.org/53039)
Short version:
test.cc: In function 'bool
std::atomic_flag_test_and_set_explicit(std::__atomic_flag_base*,
std::memory_order)':
test.cc:25:3: internal compiler error: in patch_jump_insn, at cfgrtl.c:1271
comment at cfgrtl.c:1271 suggests:
/* If the substitution doesn't succeed, die. This can happen
if the back end emitted unrecognizable instructions or if
target is exit block on some arches. */
if (!redirect_jump (as_a (insn),
block_label (new_bb), 0))
{
gcc_assert (new_bb == EXIT_BLOCK_PTR_FOR_FN (cfun));
so it's saying the backend is generating garbage.
(gdb) call debug_insn_slim(insn)
12:
{pc={(zero_extract([r23:SI],0x1,0x1)!=0)?L14:pc};zero_extract([r23:SI],0x1,0x1)=0x1;}
I've found that if I modify vax/builtins.md:
@@ -61,7 +67,7 @@
label = gen_label_rtx ();
emit_move_insn (operands[0], const1_rtx);
- emit_jump_insn (gen_jbbssi (operands[1], const0_rtx, label,
operands[1]));
+ //emit_jump_insn (gen_jbbssi (operands[1], const0_rtx, label,
operands[1]));
emit_move_insn (operands[0], const0_rtx);
emit_label (label);
DONE;
it "fixes" my internal compiler error. However, I'm not sure what is
wrong with gen_jbbssi.
My current strategy of "just changing things and seeing if they help /
comparing to 50 examples of nearly identical code" doesn't appear
to be working despite many attempts :-)
Please help, thanks.
Re: Internal compiler error building libstdc++ for vax
(updating) krister found a better hack patch which explains what the problem is, adding a useless move at the end of the instruction, so the label is not the last instruction. (And, in the problem code, the last instruction in the function.) --- a/external/gpl3/gcc/dist/gcc/config/vax/builtins.md +++ b/external/gpl3/gcc/dist/gcc/config/vax/builtins.md @@ -70,6 +70,7 @@ emit_jump_insn (gen_jbbssi (operands[1], const0_rtx, label, operands[1])); emit_move_insn (operands[0], const0_rtx); emit_label (label); + emit_move_insn (operands[0], operands[0]); DONE; }")
Re: Internal compiler error building libstdc++ for vax
It turns out (all from krister, I am still totally lost) that it is not failing for this specific reason in this case. Rather, the attached patch from krister fixes it, saying that gcc wants to change the label and then doesn't recognise the new insn thinking the memory_operand predicate is not satisfied. The new predicate is from rs6000. In retrospect the most important thing to provide was the 4 line shell script to reproduce the problem, I felt uneasy sharing that because it is with netbsd's copy of GCC (which I know how to cross-build). For the purpose of changing it to support a reversed pc/label_ref, I can probably cargo-cult make it look like branch and make the square peg fit in a round hole by a lot experimenting, but I don't understand the code I have to be changing to do that. There is this construct in the code that I don't understand why I want to do anything like it, even if I can parse what the individual parts of it mean: (set (zero_extract:SI (match_operand:QI 3 "memory_operand" "+0") (const_int 1) (match_dup 1)) (const_int 1))])] diff --git a/external/gpl3/gcc/dist/gcc/config/vax/builtins.md b/external/gpl3/gcc/dist/gcc/config/vax/builtins.md index 7be1179..5fb6da6 100644 --- a/external/gpl3/gcc/dist/gcc/config/vax/builtins.md +++ b/external/gpl3/gcc/dist/gcc/config/vax/builtins.md @@ -77,13 +77,13 @@ [(parallel [(set (pc) (if_then_else - (ne (zero_extract:SI (match_operand:QI 0 "memory_operand" "g") + (ne (zero_extract:SI (match_operand:QI 0 "volatile_mem_operand" "g") (const_int 1) (match_operand:SI 1 "general_operand" "nrm")) (const_int 0)) (label_ref (match_operand 2 "" "")) (pc))) - (set (zero_extract:SI (match_operand:QI 3 "memory_operand" "+0") + (set (zero_extract:SI (match_operand:QI 3 "volatile_mem_operand" "+0") (const_int 1) (match_dup 1)) (const_int 1))])] @@ -94,13 +94,13 @@ [(parallel [(set (pc) (if_then_else - (ne (zero_extract:SI (match_operand:HI 0 "memory_operand" "Q") + (ne (zero_extract:SI (match_operand:HI 0 "volatile_mem_operand" "Q") (const_int 1) (match_operand:SI 1 "general_operand" "nrm")) (const_int 0)) (label_ref (match_operand 2 "" "")) (pc))) - (set (zero_extract:SI (match_operand:HI 3 "memory_operand" "+0") + (set (zero_extract:SI (match_operand:HI 3 "volatile_mem_operand" "+0") (const_int 1) (match_dup 1)) (const_int 1))])] @@ -111,13 +111,13 @@ [(parallel [(set (pc) (if_then_else - (ne (zero_extract:SI (match_operand:SI 0 "memory_operand" "Q") + (ne (zero_extract:SI (match_operand:SI 0 "volatile_mem_operand" "Q") (const_int 1) (match_operand:SI 1 "general_operand" "nrm")) (const_int 0)) (label_ref (match_operand 2 "" "")) (pc))) - (set (zero_extract:SI (match_operand:SI 3 "memory_operand" "+0") + (set (zero_extract:SI (match_operand:SI 3 "volatile_mem_operand" "+0") (const_int 1) (match_dup 1)) (const_int 1))])] diff --git a/external/gpl3/gcc/dist/gcc/config/vax/predicates.md b/external/gpl3/gcc/dist/gcc/config/vax/predicates.md index 7344192..f68c3f4 100644 --- a/external/gpl3/gcc/dist/gcc/config/vax/predicates.md +++ b/external/gpl3/gcc/dist/gcc/config/vax/predicates.md @@ -109,3 +109,16 @@ (and (match_code "const_int,const_double,subreg,reg,mem") (and (match_operand:DI 0 "general_operand" "") (not (match_operand:DI 0 "illegal_addsub_di_memory_operand") + +;; Return 1 if the operand is in volatile memory. Note that during the +;; RTL generation phase, memory_operand does not return TRUE for volatile +;; memory references. So this function allows us to recognize volatile +;; references where it's safe. +(define_predicate "volatile_mem_operand" + (and (and (match_code "mem") + (match_test "MEM_VOLATILE_P (op)")) + (if_then_else (match_test "reload_completed") + (match_operand 0 "memory_operand") + (if_then_else (match_test "reload_in_progress") + (match_test "strict_memory_address_p (mode, XEXP (op, 0))") + (match_test "memory_address_p (mode, XEXP (op, 0))")
