On Oct 20, 2024, Alexandre Oliva <ol...@adacore.com> wrote:
> the x86 hits are newer, and
> the line numbers didn't help me find the errors, so I don't even know
> whether it's macros that we're talking about. These new warnings were
> encouraging, for they showed that the ifcombine incarnation of the patch
> covered additional combinations that the fold-based one couldn't. But
> they only came up in the latest version, so I know the mutually
> exclusive conditions are not neighbors.
I've looked into the error-warning in insn-attr.cc.
It's in get_attr_unit's handling of vec_dupv4sf.
(set (attr "type")
(cond [(and (eq_attr "alternative" "0")
(match_test "!TARGET_AVX2"))
(const_string "sseshuf1")
(eq_attr "alternative" "3")
(const_string "sseshuf1")
]
(const_string "ssemov")))
both cond cases, as well as the fallback value, all match the types for
unit sse. The generated code for that is the negation of the cond
conditions, followed by each of the conditions:
if ((((which_alternative != 0) || (!((!TARGET_AVX2))))
&& (which_alternative != 3))
|| ((which_alternative == 0) && ((!TARGET_AVX2)))
|| (which_alternative == 3))
return UNIT_SSE;
else
return UNIT_INTEGER;
until mergephi2, the return value is represented sort of as:
(((which_alternative != 0
|| TARGET_AVX2)
&& which_alternative != 3))
? UNIT_SSE
: (which_alternative == 0) && !TARGET_AVX2
? UNIT_SSE
: (which_alternative == 3)
? UNIT_SSE
: UNIT_INTEGER)
threadfull1 simplifies this to:
((which_alternative != 0)
? UNIT_SSE
: TARGET_AVX2
? UNIT_SSE
: !TARGET_AVX2
? UNIT_SSE
: UNIT_INTEGER)
until the field-load-merging ifcombine looks at the bit tests and
realizes they are mutually exclusive:
((which_alternative != 0)
? UNIT_SSE
: UNIT_SSE)
and that eventually gets down to UNIT_SSE, which perhaps genattrtab
could have generated to begin with.
Without the field-load merging improvement to ifcombine, we end up
taking a far more convoluted path to the same end result: phiopt2
optimizes the final ternary op to a binary op:
((which_alternative != 0)
? UNIT_SSE
: TARGET_AVX2
? UNIT_SSE
: !TARGET_AVX2 * UNIT_SSE)
(because UNIT_INTEGER is zero), then dom2 realizes that the value of
!TARGET_AVX2 is known at that point, so we get:
((which_alternative != 0)
? UNIT_SSE
: TARGET_AVX2
? UNIT_SSE
: UNIT_SSE)
reassoc1 drops the trailing test:
((which_alternative != 0)
? UNIT_SSE
: UNIT_SSE)
so does dce3, leaving only the load of which_alternative, that only dse3
drops.
So it's not quite the case that we don't realize that the conditions are
mutually exclusive, or that they are initially complementary, but the
way we take advantage of it doesn't trigger warnings.
With the new optimization, we get the warning we would have got from
fold if the conditions were neighbors. But since they only become
mutually exclusive after jump threading, fold can't deal with it, but
the improved ifcombine does, and it reports it.
Since the condition was not originally mutually exclusive, and only
became so after jump threading and various other optimizations, I
suppose we shouldn't warn any more, it could flag too many false
positives.
This also shows opportunity to improve genattrtab, that fails to notice
that all paths lead to Rome, so to speak, and outputs a reasonably
complex condition that should optimize down to a constant boolean.
--
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
