On 11/16/2016 03:57 PM, Jeff Law wrote:
On 11/02/2016 11:16 AM, Aldy Hernandez wrote:
Hi Jeff.
As discussed in the PR, here is a patch exploring your idea of ignoring
unguarded uses if we can prove that the guards for such uses are
invalidated by the uninitialized operand paths being executed.
This is an updated patch from my suggestion in the PR. It bootstraps
with no regression on x86-64 Linux, and fixes the PR in question.
As the "NOTE:" in the code states, we could be much smarter when
invalidating predicates, but for now let's do straight negation which
works for the simple case. We could expand on this in the future.
OK for trunk?
curr
commit 8375d7e28c1a798dd0cc0f487d7fa1068d9eb124
Author: Aldy Hernandez <aldyh@redhat.com>
Date: Thu Aug 25 10:44:29 2016 -0400
PR middle-end/61409
* tree-ssa-uninit.c (use_pred_not_overlap_with_undef_path_pred):
Remove reference to missing NUM_PREDS in function comment.
(can_one_predicate_be_invalidated_p): New.
(can_chain_union_be_invalidated_p): New.
(flatten_out_predicate_chains): New.
(uninit_ops_invalidate_phi_use): New.
(is_use_properly_guarded): Call uninit_ops_invalidate_phi_use.
[ snip ]
+static bool
+can_one_predicate_be_invalidated_p (pred_info predicate,
+ vec<pred_info *> worklist)
+{
+ for (size_t i = 0; i < worklist.length (); ++i)
+ {
+ pred_info *p = worklist[i];
+
+ /* NOTE: This is a very simple check, and only understands an
+ exact opposite. So, [i == 0] is currently only invalidated
+ by [.NOT. i == 0] or [i != 0]. Ideally we should also
+ invalidate with say [i > 5] or [i == 8]. There is certainly
+ room for improvement here. */
+ if (pred_neg_p (predicate, *p))
It's good enough for now. I saw some other routines that might allow us
to handle more cases. I'm OK with faulting those in if/when we see such
cases in real code.
Ok.
+
+/* Return TRUE if executing the path to some uninitialized operands in
+ a PHI will invalidate the use of the PHI result later on.
+
+ UNINIT_OPNDS is a bit vector specifying which PHI arguments have
+ arguments which are considered uninitialized.
+
+ USE_PREDS is the pred_chain_union specifying the guard conditions
+ for the use of the PHI result.
+
+ What we want to do is disprove each of the guards in the factors of
+ the USE_PREDS. So if we have:
+
+ # USE_PREDS guards of:
+ # 1. i > 5 && i < 100
+ # 2. j > 10 && j < 88
Are USE_PREDS joined by an AND or IOR? I guess based on their type it
must be IOR. Thus to get to a use #1 or #2 must be true. So to prove
we can't reach a use, we have to prove that #1 and #2 are both not true.
Right?
IOR, so yes.
+
+static bool
+uninit_ops_invalidate_phi_use (gphi *phi, unsigned uninit_opnds,
+ pred_chain_union use_preds)
+{
+ /* Look for the control dependencies of all the uninitialized
+ operands and build predicates describing them. */
+ unsigned i;
+ pred_chain_union uninit_preds[32];
+ memset (uninit_preds, 0, sizeof (pred_chain_union) * 32);
+ for (i = 0; i < MIN (32, gimple_phi_num_args (phi)); i++)
Can you replace the magic "32" with a file scoped const or #define? I
believe there's 2 existing uses of a magic "32" elsewhere in
tree-ssa-uninit.c as well.
Done.
+
+ /* Build the control dependency chain for `i'... */
+ if (compute_control_dep_chain (find_dom (e->src),
+ e->src,
+ dep_chains,
+ &num_chains,
+ &cur_chain,
+ &num_calls))
Does this miss the control dependency carried by E itself.
ie, if e->src ends in a conditional, shouldn't that conditional be
reflected in the control dependency chain as well? I guess we'd have to
have the notion of computing the control dependency for an edge rather
than a block. It doesn't look like compute_control_dep_chain is ready
for that. I'm willing to put that into a "future work" bucket.
Hmmm, probably not. So yeah, let's put that in the future work bucket :).
So I think just confirming my question about how USE_PREDS are joined at
the call to uninit_opts_invalidate_phi_use and fixing the magic 32 to be
a file scoped const or a #define and this is good to go on the trunk.
I've retested with no regressions on x86-64 Linux.
OK for trunk?