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;
