Re: [PATCH] warn on returning alloca and VLA (PR 71924, 90549)
On 6/26/19 6:11 PM, Jeff Law wrote: On 6/18/19 9:19 PM, Martin Sebor wrote: On 6/14/19 2:59 PM, Jeff Law wrote: [ big snip ] A COND_EXPR on the RHS of an assignment is valid gimple. That's what we need to consider here -- what is and what is not valid gimple. And its more likely that PHIs will be transformed into RHS COND_EXPRs -- that's standard practice for if-conversion. Gosh, how to get one? It happens all the time :-) Since I know DOM so well, I just shoved a quick assert into optimize_stmt to abort if we encounter a gimple assignment where the RHS is a COND_EXPR. It blew up instantly building libgcc :-) COmpile the attached code with -O2 -m32. I wasn't saying it's not valid Gimple, just that I hadn't seen it come up here despite compiling Glibc and the kernel with the patched GCC. The only codes I saw are these: addr_expr, array_ref, bit_and_expr, component_ref, max_expr, mem_ref, nop_expr, parm_decl, pointer_plus_expr, ssa_name, and var_decl The only one here that's really surprising is the MAX_EXPR. But it is what it is. What I was asking for is a test case that makes COND_EXPR come up in this context. But I managed to find one by triggering the ICE with GDB. The file reduced to the following test case: Sorry. email can be a tough medium to nail down specific details. extern struct S s; // S has to be an incomplete type void* f (int n) { void *p; int x = 0; for (int i = n; i >= 0; i--) { p = if (p == (void*)-1) x = 1; else if (p) return p; } return x ? (void*)-1 : 0; } and the dump: [local count: 59055800]: # x_10 = PHI <1(5), 0(2)> _5 = x_10 != 0 ? -1B : 0B; [local count: 114863532]: # _3 = PHI <(4), _5(6), (3)> return _3; It seems a little odd that the COND_EXPR disappears when either of the constant addresses becomes the address of an object (or the result of alloca), and also when the number of iterations of the loop is constant. That's probably why it so rarely comes up in this context. Going into phiopt2 we have: ;; basic block 6, loop depth 0 ;;pred: 5 if (x_1 != 0) goto ; [71.00%] else goto ; [29.00%] ;;succ: 8 ;;7 ;; basic block 7, loop depth 0 ;;pred: 6 ;;succ: 8 ;; basic block 8, loop depth 0 ;;pred: 3 ;;7 ;;6 # _3 = PHI <(3), 0B(7), -1B(6)> return _3; The subgraph starting at block #6 is a classic case for turning branchy code into straightline code using a COND_EXPR on the RHS of an assignment. So you end up with something like this: ;; basic block 6, loop depth 0 ;;pred: 5 _5 = x_1 != 0 ? -1B : 0B; ;;succ: 7 ;; basic block 7, loop depth 0 ;;pred: 3 ;;6 # _3 = PHI <(3), _5(6)> return _3; Now for this specific case within phiopt we are limited to cases there the result is 0/1 or 0/-1. That's why you get something different when you exchange one of the constants for the address of an object, or anything else for that matter. This is all a bit academic -- the key point is that we can have a COND_EXPR on the RHS of an assignment. That's allowed by gimple. Sadly this is also likely one of the places where target characteristics come into play -- targets define a BRANCH_COST which can significantly change the decisions for the initial generation of conditionals. It's one of the things that makes writing tests for jump threading, if conversion and other optimizations so damn painful -- on one target we'll have a series of conditional jumps, on anothers we may have a series of logicals, potentially with COND_EXPRs. That said, even though I've seen a few references to COND_EXPR in the midle-end I have been assuming that they, in general, do get transformed into PHIs. But checking the internals manual I found in Section 12.6.3 this: A C ?: expression is converted into an if statement with each branch assigning to the same temporary. ... The GIMPLE level if-conversion pass re-introduces ?: expression, if appropriate. It is used to vectorize loops with conditions using vector conditional operations. This GDB test case is likely the result of this reintroduction. Nope. It happens much earlier in the pipeline :-) And in a more general sense, this kind of permissiveness is not future proof. What happens if someone needs to add another EXPR node that is valid on the RHS where such recursion is undesirable? How are they supposed to know that we've got this permissive recursive call and that it's going to do the wrong thing? And if it's an EXPR node with no arguments, then we're going to do a read out of the bounds of the object and all bets are off at that point (yes we have zero operand EXPR nodes, but thankfully I don't think they should up in the contexts you care about). Sure. When things are hardcoded
[PATCH, i386]: Move MMX abs pattern outside normal optabs namespace
As explained on top of mmx.md, MMX patterns should be defined outside the normal optabs namespace. 2019-06-30 Uroš Bizjak * config/i386/sse.md (ssse3_abs2): Rename from abs2. (abs2): New expander. * config/i386/i386-builtin.def (__builtin_ia32_pabsb): Use CODE_FOR_ssse3_absv8qi2. (__builtin_ia32_pabsw): Use CODE_FOR_ssse3_absv4hi2. (__builtin_ia32_pabsd): Use CODE_FOR_ssse3_absv2si2. Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}. Committed to mainline SVN. Uros. Index: config/i386/i386-builtin.def === --- config/i386/i386-builtin.def(revision 272833) +++ config/i386/i386-builtin.def(working copy) @@ -830,11 +830,11 @@ /* SSSE3 */ BDESC (OPTION_MASK_ISA_SSSE3, 0, CODE_FOR_absv16qi2, "__builtin_ia32_pabsb128", IX86_BUILTIN_PABSB128, UNKNOWN, (int) V16QI_FTYPE_V16QI) -BDESC (OPTION_MASK_ISA_SSSE3 | OPTION_MASK_ISA_MMX, 0, CODE_FOR_absv8qi2, "__builtin_ia32_pabsb", IX86_BUILTIN_PABSB, UNKNOWN, (int) V8QI_FTYPE_V8QI) +BDESC (OPTION_MASK_ISA_SSSE3 | OPTION_MASK_ISA_MMX, 0, CODE_FOR_ssse3_absv8qi2, "__builtin_ia32_pabsb", IX86_BUILTIN_PABSB, UNKNOWN, (int) V8QI_FTYPE_V8QI) BDESC (OPTION_MASK_ISA_SSSE3, 0, CODE_FOR_absv8hi2, "__builtin_ia32_pabsw128", IX86_BUILTIN_PABSW128, UNKNOWN, (int) V8HI_FTYPE_V8HI) -BDESC (OPTION_MASK_ISA_SSSE3 | OPTION_MASK_ISA_MMX, 0, CODE_FOR_absv4hi2, "__builtin_ia32_pabsw", IX86_BUILTIN_PABSW, UNKNOWN, (int) V4HI_FTYPE_V4HI) +BDESC (OPTION_MASK_ISA_SSSE3 | OPTION_MASK_ISA_MMX, 0, CODE_FOR_ssse3_absv4hi2, "__builtin_ia32_pabsw", IX86_BUILTIN_PABSW, UNKNOWN, (int) V4HI_FTYPE_V4HI) BDESC (OPTION_MASK_ISA_SSSE3, 0, CODE_FOR_absv4si2, "__builtin_ia32_pabsd128", IX86_BUILTIN_PABSD128, UNKNOWN, (int) V4SI_FTYPE_V4SI) -BDESC (OPTION_MASK_ISA_SSSE3 | OPTION_MASK_ISA_MMX, 0, CODE_FOR_absv2si2, "__builtin_ia32_pabsd", IX86_BUILTIN_PABSD, UNKNOWN, (int) V2SI_FTYPE_V2SI) +BDESC (OPTION_MASK_ISA_SSSE3 | OPTION_MASK_ISA_MMX, 0, CODE_FOR_ssse3_absv2si2, "__builtin_ia32_pabsd", IX86_BUILTIN_PABSD, UNKNOWN, (int) V2SI_FTYPE_V2SI) BDESC (OPTION_MASK_ISA_SSSE3, 0, CODE_FOR_ssse3_phaddwv8hi3, "__builtin_ia32_phaddw128", IX86_BUILTIN_PHADDW128, UNKNOWN, (int) V8HI_FTYPE_V8HI_V8HI) BDESC (OPTION_MASK_ISA_SSSE3 | OPTION_MASK_ISA_MMX, 0, CODE_FOR_ssse3_phaddwv4hi3, "__builtin_ia32_phaddw", IX86_BUILTIN_PHADDW, UNKNOWN, (int) V4HI_FTYPE_V4HI_V4HI) Index: config/i386/sse.md === --- config/i386/sse.md (revision 272834) +++ config/i386/sse.md (working copy) @@ -16584,7 +16584,7 @@ } }) -(define_insn "abs2" +(define_insn "ssse3_abs2" [(set (match_operand:MMXMODEI 0 "register_operand" "=y,Yv") (abs:MMXMODEI (match_operand:MMXMODEI 1 "register_mmxmem_operand" "ym,Yv")))] @@ -16599,6 +16599,12 @@ (set (attr "prefix_rex") (symbol_ref "x86_extended_reg_mentioned_p (insn)")) (set_attr "mode" "DI,TI")]) +(define_insn "abs2" + [(set (match_operand:MMXMODEI 0 "register_operand") + (abs:MMXMODEI + (match_operand:MMXMODEI 1 "register_operand")))] + "TARGET_MMX_WITH_SSE && TARGET_SSSE3") + ; ;; ;; AMD SSE4A instructions
Re: Fail building modula2.
Gaius, I missed the fact that there are two patch sets.?? Things built like a charm.?? Testing now. Thank you. Ed
Re: [PATCH, Ada] Push -shared-libgcc where needed.
> 2019-06-30 Iain Sandoe > > * gnatlink.adb (Link_Step): Remove duplicate -static-libgcc switches. > Push -shared-libgcc explicitly, when it is the target default (unless > overidden by the static flag). > When the user has put an instance of shared/static-libgcc do not push > a duplicate of this. OK for mainline, thanks. -- Eric Botcazou
[PATCH, Ada] Push -shared-libgcc where needed.
Hi Eric, Gnatlink has code that checks for duplicate '-shared-libgcc’ switches (but not duplicate ‘static-libgcc’) and also pushes ’static-libgcc' onto the link line for targets that default to static linking, provided '-shared-libgcc' is not present. For targets that should use a shared libgcc we need the same process to be applied (in inverse), in the event that they do not default to providing the shared flag implicitly***. So this adds the complementary set of tests for the shared case and pushes the shared flag as needed. As a minor tidy-up there’s no need push duplicates of the libgcc switch onto the link line when one has already been seen (given by the user). The patch does not alter any of the platform defaults for static/shared libgcc, but it ensures that the intent of the link is explicit. OK for trunk? thanks Iain *** I have patches in progress to resolve the long-standing unwinder issues on Darwin, and one consequence of those will be to make the libgcc linkage default to "static" unless -f{,objc-}exceptions is present (or it is overidden with the shared flag). Once that tidy-up is complete, then it will be possible to switch all Darwin >= 10 (10.6) to the same defaults as Linux (since the unwinder is in libSystem not libgcc_s). === gcc/ada/ 2019-06-30 Iain Sandoe * gnatlink.adb (Link_Step): Remove duplicate -static-libgcc switches. Push -shared-libgcc explicitly, when it is the target default (unless overidden by the static flag). When the user has put an instance of shared/static-libgcc do not push a duplicate of this. diff --git a/gcc/ada/gnatlink.adb b/gcc/ada/gnatlink.adb index e8a1b92..5e5ede0 100644 --- a/gcc/ada/gnatlink.adb +++ b/gcc/ada/gnatlink.adb @@ -1884,6 +1884,7 @@ begin Clean_Link_Option_Set : declare J : Natural; Shared_Libgcc_Seen : Boolean := False; + Static_Libgcc_Seen : Boolean := False; begin J := Linker_Options.First; @@ -1905,7 +1906,7 @@ begin end if; end if; --- Remove duplicate -shared-libgcc switch +-- Remove duplicate -shared-libgcc switches if Linker_Options.Table (J).all = Shared_Libgcc_String then if Shared_Libgcc_Seen then @@ -1919,6 +1920,20 @@ begin end if; end if; +-- Remove duplicate -static-libgcc switches + +if Linker_Options.Table (J).all = Static_Libgcc_String then + if Static_Libgcc_Seen then + Linker_Options.Table (J .. Linker_Options.Last - 1) := +Linker_Options.Table (J + 1 .. Linker_Options.Last); + Linker_Options.Decrement_Last; + Num_Args := Num_Args - 1; + + else + Static_Libgcc_Seen := True; + end if; +end if; + -- Here we just check for a canonical form that matches the -- pragma Linker_Options set in the NT runtime. @@ -1950,14 +1965,27 @@ begin -- libgcc, if gcc is not called with -shared-libgcc, call it -- with -static-libgcc, as there are some platforms where one -- of these two switches is compulsory to link. +-- Don't push extra switches if we already saw one. if Shared_Libgcc_Default = 'T' and then not Shared_Libgcc_Seen + and then not Static_Libgcc_Seen then Linker_Options.Increment_Last; Linker_Options.Table (Linker_Options.Last) := Static_Libgcc; Num_Args := Num_Args + 1; end if; + +-- Likewise, the reverse. + +if Shared_Libgcc_Default = 'H' + and then not Static_Libgcc_Seen + and then not Shared_Libgcc_Seen +then + Linker_Options.Increment_Last; + Linker_Options.Table (Linker_Options.Last) := Shared_Libgcc; + Num_Args := Num_Args + 1; +end if; end if; end Clean_Link_Option_Set;