Re: [PATCH] dse: Handle SUBREGs of word REGs differently for WORD_REGISTER_OPERATIONS targets [PR109040]

[Jeff Law <jeffreyalaw@gmail.com>]

On 4/5/23 10:48, Jakub Jelinek wrote:
> On Wed, Apr 05, 2023 at 10:17:59AM -0600, Jeff Law wrote:
>>> It is true that an instruction like
>>> (insn 8 7 9 2 (set (reg:HI 141)
>>>           (subreg:HI (reg:SI 142) 0)) "aauu.c":6:18 181 {*movhi_internal}
>>>        (nil))
>>> can appear in the IL on WORD_REGISTER_OPERATIONS target, but I think the
>>> upper bits shouldn't be random garbage in that case, it should be zero
>>> extended or sign extended.
>> Well, that's one of the core questions here.  What are the state of the
>> upper 16 bits of (reg:HI 141)?  The WORD_REGISTER_OPERATIONS docs aren't
>> 100% clear as we're not really doing any operation.
>>
>> So again, I think we need to decide if the DSE transformation is correct or
>> not.  I *think* we can aggree that insn 39 is OK.  It's really the semantics
>> of insn 47 that I think we need to agree on.  What is the state of the upper
>> 16 bits of (reg:HI 175) after insn 47?
> 
> I'm afraid I don't know the answers here, I think Eric is
> WORD_REGISTER_OPERATIONS expert here I think these days (most of the major
> targets are !WORD_REGISTER_OPERATIONS).
Hopefully he'll chime in.

> Intuitively, WORD_REGISTER_OPERATIONS from the description would be
> that
> (insn 47 35 39 2 (set (reg:HI 175)
>          (subreg:HI (reg:SI 166) 0)) "pr109040.c":9:11 179 {*movhi_internal}
>       (expr_list:REG_DEAD (reg:SI 166)
>          (nil)))
> would copy not just the low 16-bits of pseudo 166 to 175, but also the upper
> 16-bits.  
I've gone back and forth repeatedly on this.

Originally I didn't really see this as an operation.  But the more and 
more I ponder it feels like it's an operation and thus should be subject 
to WORD_REGISTER_OPERATIONS.

While it's not really binding on RTL semantics, if we look at how some 
architectures implement reg->reg copies, they're actually implemented 
with an ADD or IOR -- so a real operation under the hood.

If we accept a subreg copy as an operation and thus subject to 
WORD_REGISTER_OPERATIONS then that would seem to imply the combine is 
the real problem here.  Otherwise dse is the culprit.







But if that is so, then something is broken in the code below.
> Some archeology shows that we were doing that on all arches initially
> and then
> Wed May 13 17:38:35 1998  Richard Kenner  <kenner@vlsi1.ultra.nyu.edu>
> 
> 	* combine.c (simplify_comparison, case AND): Don't commute AND
> 	with SUBREG if constant is whole mode and don't do if lowpart
> 	and not WORD_REGISTER_OPERATIONS.
> restricted that to WORD_REGISTER_OPERATIONS only.
> The whole optimization is then likely
> Wed Mar 18 05:54:25 1998  Richard Kenner  <kenner@vlsi1.ultra.nyu.edu>
> 
> 	* combine.c (gen_binary): Don't make AND that does nothing.
> 	(simplify_comparison, case AND): Commute AND and SUBREG.
> 	* i386.h (CONST_CONSTS, case CONST_INT): One-byte integers are cost 0.
> but the code has been tweaked further many times since then.
Sigh.  1998 and probably directly from Kenner's tree at the time.  So no 
public discussion, likely no testcases either.  Off to my private 
archives.  Yup, no discussion of Kenner's patch or testcase.

Interestingly enough I did stumble across Andreas Schwab reporting a bug 
in Kenner's change, though on the m68k and unrelated to 
WORD_REGISTER_OPERATIONS


jeff