Eric Botcazou <ebotca...@adacore.com> writes:
>> Combine is asking simplify-rtx.c to truncate an addition to QImode
>> and simplify-rtx.c is providing a reasonable representation of that.
>> It's the representation we should use when matching against .md patterns,
>> for example. The problem is that combine doesn't want to keep the
>> truncation in this case, but doesn't know that yet.
>
> I disagree, I don't find it reasonable to turn an addition in SImode into an
> addition in QImode when the machine supports the former but not the latter.
> I agree that it may help in some contexts, but then the transformation should
> be restricted to these contexts.
>
>> Right, but the only complaint I know of is about its effect on combine.
>> And my point is that that complaint isn't about combine failing to combine
>> instructions per se. It's that combine is failing to remove a redundant
>> operation. With the right input, the same rtl sequence could conceivably
>> be generated on a CISC target like x86_64, since it defines all the required
>> patterns (SImode addition, QI->SI zero extension, SImode comparison). It
>> could also occur with a sequence that starts out as a QImode addition. So
>> trying to make the simplification depend on CISCness seems like papering
>> over the problem.
>
> The problem is that this changes the combinations tried by the combiner in a
> way that is adverse to most RISC targets. Sure, we could change all the
> affected back-ends but what's the point exactly? What do we gain here?
>
> Look for example at comment #4 in PR rtl-optimization/58295.
The comment says that we're trying to match:
1. (set (reg:SI) (zero_extend:SI (plus:QI (mem:QI) (const_int))))
2. (set (reg:QI) (plus:QI (mem:QI) (const_int)))
3. (set (reg:QI) (plus:QI (subreg:QI) (const_int)))
4. (set (reg:CC) (compare:CC (subreg:QI) (const_int)))
5. (set (reg:CC) (compare:CC (plus:QI (mem:QI) (const_int))))
6. (set (reg:SI) (leu:SI (subreg:QI) (const_int)))
7. (set (reg:SI) (leu:SI (subreg:QI) (const_int)))
8. (set (reg:SI) (leu:SI (plus:QI ...)))
And I think that's what we should be matching in cases where the
extension isn't redundant, even on RISC targets.
The problem here isn't really about which mode is on the plus,
but whether we recognise that the extension instruction is redundant.
I.e. we start with:
(insn 9 8 10 2 (set (reg:SI 120)
(plus:SI (subreg:SI (reg:QI 118) 0)
(const_int -48 [0xffffffffffffffd0]))) test.c:6 -1
(nil))
(insn 10 9 11 2 (set (reg:SI 121)
(and:SI (reg:SI 120)
(const_int 255 [0xff]))) test.c:6 -1
(nil))
(insn 11 10 12 2 (set (reg:CC 100 cc)
(compare:CC (reg:SI 121)
(const_int 9 [0x9]))) test.c:6 -1
(nil))
and what we want combine to do is to recognise that insn 10 is redundant
and reduce the sequence to:
(insn 9 8 10 2 (set (reg:SI 120)
(plus:SI (subreg:SI (reg:QI 118) 0)
(const_int -48 [0xffffffffffffffd0]))) test.c:6 -1
(nil))
(insn 11 10 12 2 (set (reg:CC 100 cc)
(compare:CC (reg:SI 120)
(const_int 9 [0x9]))) test.c:6 -1
(nil))
But insn 11 is redundant on all targets, not just RISC ones.
It isn't about whether the target has a QImode addition or not.
>> If you think the patch was wrong or if you feel the fallout is too great
>> then please feel free to revert it.
>
> I think that the fallout is too great for RISC targets, yes, so I'm trying to
> find a reasonable compromise.
Well, I think making the simplify-rtx code conditional on the target
would be the wrong way to go. If we really can't live with it being
unconditional then I think we should revert it. But like I say I think
it would be better to make combine recognise the redundancy even with
the new form. (Or as I say, longer term, not to rely on combine to
eliminate redundant extensions.) But I don't have time to do that myself...
Thanks,
Richard
