On Dec 3, 2024, Richard Biener <richard.guent...@gmail.com> wrote:
> On Fri, Nov 22, 2024 at 10:22 PM Alexandre Oliva <ol...@adacore.com> wrote:
>> + /* Identify the load, if there is one. */
>> + if (TREE_CODE (exp) == SSA_NAME
>> + && !SSA_NAME_IS_DEFAULT_DEF (exp))
>> + {
>> + gimple *def = SSA_NAME_DEF_STMT (exp);
>> + if (gimple_assign_load_p (def))
>> + {
>> + loc[3] = gimple_location (def);
>> + *load = def;
>> + exp = gimple_assign_rhs1 (def);
>> + }
>> + }
> If it's not a load why would we continue here?
I've seen cases among the added tests in which this folder could combine
expressions that other ifcombiners couldn't, if only it wouldn't insist
on loads. IIRC it had to do with shifted or xored compares.
> Soo - we probably misunderstood us here. Rather than rewriting the
> individual matching steps into (match ...) I thought we'd want (match ...)
> for the entire set of expression forms we like to cover here.
Yeah, I got that intent, I just couldn't figure out a way to implement
that with the flexibility needed to handle xor's left- or right-hand
operand.
>> + tree ret = force_gimple_operand_gsi (&gsi, ref,
>> + true, NULL_TREE,
>> + true, GSI_NEW_STMT);
>> + /* We know the vuse is supposed to end up being the same as that at the
>> + original load at the insertion point, but if we don't set it, it will
>> be a
>> + generic placeholder that only the global SSA update at the end of the
>> pass
>> + would make equal, too late for us to use in further combinations. So
>> go
>> + ahead and copy the vuse. */
>> + use_operand_p use_p = gimple_vuse_op (gsi_stmt (gsi));
> So you're relying on force_gimple_operand to get you exactly the load as
> last new stmt - but it might constant fold from an initializer or
> insert a conversion
> as last stmt.
Aah, nice, good catch! Will fix.
> So I think you
> instead want to either re-implement make_bit_field_ref to generate GIMPLE
> (ok, probably not ...), or change the above to gimplify into a gimple_seq
> and ideally simply use gsi_replace_with_seq_vops in case 'point' is to
> be replaced? Alternatively you can factor out the "head" of
> gsi_replace_with_seq_vops and take reaching_vuse and location as arg
> and insert the sequence before 'point', preserving it.
ifcombine hasn't replaced preexisting defs, and we don't insist in a
single use (maybe we should), so we have to leave preexisting loads
alone, hoping for later dead code elimination.
>> + for (int i = 0; i < 2; i++)
>> + {
>> + tree type = lang_hooks.types.type_for_size (bitsiz[i], 1);
> this can return NULL?
I didn't think so, when using integral modes, but I guess it doesn't
hurt to play safe. It's too late at that point to give up on the
transformation, though, so I'll just assert.
> In fact type_for_mode might be more appropriate. Both langhooks might
> not be fully implemented for all frontends though, type_for_mode is
> more commonly used.
'k
>> + if (TREE_SIDE_EFFECTS (ll_arg) || TREE_SIDE_EFFECTS (lr_arg)
>> + || TREE_SIDE_EFFECTS (rl_arg) || TREE_SIDE_EFFECTS (rr_arg))
>> + return 0;
> As you control the API entry, this (args should be SSA name or constant,
> definitely is_gimple_val) ...
Yeah, that and the TRUTH_*IF_EXPR handling are left over from pre-gimple
fold.
>> + if (get_best_mode (end_bit - first_bit, first_bit, 0, 0,
>> + TYPE_ALIGN (TREE_TYPE (ll_inner)), BITS_PER_WORD,
>> + volatilep, &lnmode))
> As to above, get_best_mode doesn't ensure there's a type_for_size/mode
> for the mode returned.
*nod* (see below)
>> + lntype = lang_hooks.types.type_for_size (lnbitsize, 1);
>> + if (!lntype)
>> + {
>> + gcc_checking_assert (l_split_load);
>> + lntype = build_nonstandard_integer_type (lnbitsize, 1);
> You're falling back to build_nonstandard_integer_type here
> (you can use that unconditionally just fine, btw)
'k, I'm moving that out of the if, and adding code to give up if we
can't get a suitable type, here and at rntype.
>> + warning (0, "comparison is always %d", wanted_code == NE_EXPR);
> you copied these from fold-const.cc, but as you have ll_loc for example you
> want to at least use warning_at (...). I wonder if we have a good fitting
> option to use as well so the warning can be suppressed.
> -Wtautological-compare
> maybe fits (it's marked C/C++ only though, would need to get Common).
Will do.
Thanks,
--
Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/
Free Software Activist GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive
