Re: [RFA:] Don't strip unary ops when matching constraints. (PRtarget/23424)

[Hans-Peter Nilsson <hans-peter dot nilsson at axis dot com>]

> Date: Tue, 18 Oct 2005 17:09:35 +0100
> From: Joern RENNECKE <joern.rennecke@st.com>

> Hans-Peter Nilsson wrote:
> >> And what 
> >>code base
> >>were you intending to use for the comparison?
> >Just a regular build and test.
> A regular cross-build wouldn't include any gcc code for the target.

Is gcc of particular interest as an application hosted on SH and
SH64?

>  The
> libraries have some value for a regression test, but they are not as
> complex as full applications.

*shrug*   I don't know what kind of code generation regressions
that would be a concern, and that you expect to see with
application builds, that wouldn't be *noticed* by in generated
assembled code for a regular build and test run.

> If you plan on making a similar patch to the original one for 4.2, we still
> have to define what a meaningful test is.

I'm not sure I follow here, unless by a "similar patch" you mean
to just remove the code.  FWIW, I don't plan on changing any of
this for 4.2, except I maybe to help having the code be
*removed*, not guarded by a macro.  I guess I could be in
assistance of that happening, but still, comparing target
libraries and assembling testsuite results should be sufficient.

> >An undocumented quirk should not be called an interface.
> The fact that it is not documented may be just be a documentation bug.

Um, it has fundamental flaws besides not being documented, see below.

> >I've already said why: It's not documented, and unary operators
> >are the only ones that are stripped.  
> >
> You couldn't meaningfully strip a binary operator to arrive at a single
> operand.

Neither can you silently strip an unop and pretend that's the
operand you want to look at!  Anyway, the point is that treating
some RTX as a special case and special-casing that all around
e.g. reload just leads to breakage.

> >It causes for example
> >(sign_extend:SI (mem:QI ...)) to be passed as (mem:QI ...) to
> >the constraint-test so it will not match; a constraint for an
> >unop will never see the same as the predicate.  Just plain
> >bogus.
> >  
> It does serve a purpose.  If you have a pattern that accepts unops on 
> registers,
> a port using the current interface will have a constraint sepcifying a 
> register
> class.
> If a  pseudo register is used which ends up in a stack slot, reload will see
> the register class constraint and know how to reload the inside of the unop.
> The underlying problem is that there is actually no way to tell reload how
> you want operands relaoded.  You could make a pattern where the unop is
> not inside the operand, but then it couldn't match at the same time 
> unop-free
> registers.

Your constraints can't match both an unop and the typ of operand
of the unop with a different alternative; both must be at the
same alternative for the pattern.  You can't match different
unops with different alternatives, if they contain the same type
of operand.  Those restrictions IMO pose a bigger problem than
not being able to guide reload for whatever is inside the unop:
if you need that, you can do it with a separate pattern where
the unop is explicit rather than hidden in an operand.

It doesn't seem to work the way you hope anyway.  See below.
(That adds yet another restriction: you always have to allow
*both* the unop and its inner operand for any alternative.  So
why not have a constraint matching the unop separate and
explicit?)

>  There is actually a related issue with SUBREGS, i.e. should the
> inside of the SUBREG be reloaded, or the entire SUBREG; different ports
> actually want different things - and not necessarily the same in all 
> instances
> either.  A proper solution should include an interface to tell reload 
> how you
> want operands with unops and/or SUBREGS reloaded.

Without details on what problem you have in mind here, I'd say,
the whole SUBREG should be reloaded, presumably to a REG (but a
stack slot if only MEM fits).  If you have to special-case the
inside of a SUBREG, there's something that's not expressed
correctly and the whole problem needs to be fixed another way.

> I think we basically want a hook which looks at a constraint alternative and
> tells reload if a given operand can be reloaded to match this 
> alternative, and
> if so, which part of the operand actually has to be reloaded, and...

That idea seems to have merits, as a future separate change I
should say.

> >>I didn't expect this unop -stripping ad hoc, but I've encountered it and
> >>dealt with it.  And subsequently I expected it.
> >
> >I can see that, now that I looked further at the SH port.  I'm
> >kind of worried that you didn't bring up this surprise when you
> >encountered it.  It should be fixable anyway by adding proper
> >constraints for the unops to the SH port.
> >
> I can make constraints so that the unops keep being accepted by the 
> constraints,
> but as these additional constraints would not be register class 
> constraint, they
> could not guide reload how to do its work.

The reload issue you want to express seems to be whether for a
(container:M (unop:M (reg:N pseudo))) you'd get
1: (container:M (reg-or-mem:M)) or, as it seems you hope
2: (container:M (unop:M (reg-or-mem:M))).

It seems *as long as things work without the patch* you'd
*already* get (1), so no difference.  I'm interested in proving
this to you.

IMO this unop-stripping is too messed up to be labeled as
anything else than a bug.  The unop-stripping is just local when
the particular operand is tested; recog_data.operand[] isn't
adjusted.  The patch I sent changes the functions
constrain_operands and find_reloads, but nowhere else.  Observe
that subst_reloads doesn't check for unop, so a reload will
replace at the location of the original operand; it will replace
the whole unop, not the inner operand.  At least by code
inspection.

To wit, for a (container:M (unop:M (reg:N Pseudo))), the
constraints will be matched against (reg:N Pseudo) but if they
don't match, the whole (unop:M (reg:N Pseudo)) will be replaced,
and you *have* to accept a register alternative besides the
unop.  It sounds like you didn't expect that.

Did I miss anything?  I don't see any other related unop
adjustments or handling.  For example, I didn't have to fix
find_reloads unop-stripping for digit-constraints (the
same-as-the-Nth-operand ones) like I did for constrain_operands,
*because that adjustment is already missing*.  Well, isn't that
a bug right there!  At least for offsettable mems when unops are
used with digit-constraints, if anyone uses them IIUC.

> >Is your point that predicate-less
> >operands *with* constraints would seem an indication of a bug?

(Typo, I meant to write "would *not* seem", for the record.)

> No, my point is that this is a potential issue that you won't find by 
> looking
> for predicates that accept unops.

I think I lost you here.  Can you make up an example of what you
mean?  The unop must certainly pass a predicate test (when there
is one) for it to be able to be stripped before matching a
constraint.

Random unops being stripped at constraint checking would be a
big problem indeed. :-)

> >How many are there?  Is there something worth discussing here?
> >
> I don't know how many there are.  Obviously, thre are lots of expanders
> which don't have full (or any) operand predicates, so a simple-minded
> grep wouldn't give you meaningful information.

I looked at each define_predicate that matches an unops, *then*
at the uses.  It seems to me I'd only have to look for operands
with *empty* predicates to catch the extra cases you mention.
What's wrong with that check?  Please be explicit: "potential
issue" just seems FUDish.  And I haven't seen any target that
*generates* define_predicate yet.  Your other
md-generated-on-the-fly argument seems generally dismissible:
besides arm and h8300 generating .md contents, do you see any
that I don't see?

Let's turn the question around, let's assume there are ports
that I wouldn't catch with that kind of search: are you sure
*they* want to have the unop-stripping-for-constraint?  Even the
people adding this stripping refer to it as hackery that can go
away rather than a useful interface.

Anyhow, I checked the empty-predicate with non-empty constraint
cases, using grep -A1 match_operand */*.md and then a
regexp-isearch 'match_operand[^(]*""[^"(]"[^"]' in emacs, then
pruned all cases with unsuspect constraints and call insns.
There's a (set (...)  (match_operand:DF 1 "" "?F,m")) and
similar :SF in pa.md which look a bit suspicious but if an unop
would sneak in there, it seems it'd get caught as invalid
assembly code.  It certainly doesn't seem intended that an unop
should get there; a comment says it's for reloading
const_doubles.  (Yeah, I saw and_shl_scratch in sh.md, but it's
not suspect.)  So, nothing there.

> >Right, besides the wrong assumption on sh64 being built as a
> >multilib of sh-elf, I didn't go further on checking SH once I'd
> >identified it as "affected".  I didn't dream of the port being
> >adjusted with no screaming comment the unexpected "interface".
> >
> There were lots of other things to worry about that were obviously wrong,
> e.g. TRULY_NOOP_TRUNCATION ignored in lots of places.
> I don't think the details about how instruction recognition and reload work
> have ever been 100% documented.  We generally try to improve things, but
> we can't get everything perfect at once.

But we can *at least* fix things *when we notice them*, not just
throw up our hands and make up reasons to keep whatever flawed
code is there as it is.

> >Anyway, the bottom line is that you proposed solving this by a
> >hook.  I've accepted that route, though reluctantly.  Mark
> >Mitchell agreed to solving it by a hook, so I guess the next
> >thing is for me to adjust my patch.
> >
> Yes, that's fine with me.  However, there was also talk about removing the
> hook for 4.2, and if/when we want to do that, we have to consider carefully
> all the implications.

Beyond what we've covered here, what?  I think I've brought up
enough correctness issues, and shown that there are no benefits
(and I hope to show it further, stay tuned).  I hope that trumps
other implications...  I hope I could help fixing the SH64 port
but I'm not inclined to do any beyond making sure it emits the
same code for a build and test run.  Could you help with further
testing?

Oh, and BTW, I compared the 34112 asm files that passes through
the target assembler as part of a sh-elf+sh-sim build and test
run (there are certainly more compiled, but they aren't
*assembled*).  There were no code changes there.  There were
lots of (un)interesting differences regarding output of debug
and constants and of course the anon namespace
random-name-kludge, but no code changes at all.

I hope to contribute the asmwrap script and some
--enable-asm-wrapper=... option to go with it, some time.

brgds, H-P