[PATCH] R600/SI: Fix various operand legalization bugs

[Matt Arsenault]

On Sep 26, 2014, at 10:43 AM, Tom Stellard <tom at stellard.net> wrote:

> On Thu, Sep 25, 2014 at 05:36:41PM -0700, Matt Arsenault wrote:
>> 
>> On Sep 25, 2014, at 12:40 PM, Tom Stellard <tom at stellard.net> wrote:
>> 
>>>> From 50bc914204a37c7f314d65ca8a50581f3935e390 Mon Sep 17 00:00:00 2001
>>>> From: Matt Arsenault <Matthew.Arsenault at amd.com>
>>>> Date: Mon, 22 Sep 2014 19:30:31 -0400
>>>> Subject: [PATCH 01/10] R600/SI: Fix using wrong operand indices when commuting
>>>> 
>>>> No test since the current SIISelLowering::legalizeOperands
>>>> effectively hides this, and the general uses seem to only fire
>>>> on SALU instructions which don't have modifiers between
>>>> the operands.
>>>> 
>>>> When trying to use legalizeOperands immediately after
>>>> instruction selection, it now sees a lot more patterns
>>>> it did not see before which break on this.
>>> 
>>> I have a few comments, I would also like to test the final version of
>>> these before they are committed.
>> 
>> New versions attached, although I’ve only changed comments and things like that
>> 
>>> 
>>>> From 679655e6f32ce0091bce473eb8251c41ad76a7d8 Mon Sep 17 00:00:00 2001
>>>> From: Matt Arsenault <Matthew.Arsenault at amd.com>
>>>> Date: Mon, 22 Sep 2014 20:29:12 -0400
>>>> Subject: [PATCH 03/10] R600/SI: Don't move operands that are required to be
>>>> SGPRs
>>>> 
>>>> e.g. v_cndmask_b32 requires the condition operand be an SGPR.
>>>> If one of the source operands were an SGPR, that would be considered
>>>> the one SGPR use and the condition operand would be illegally moved.
>>>> ---
>>>> lib/Target/R600/SIInstrInfo.cpp | 21 ++++++++++++++++++++-
>>>> test/CodeGen/R600/v_cndmask.ll  | 42 ++++++++++++++++++++++++++++++++---------
>>>> 2 files changed, 53 insertions(+), 10 deletions(-)
>>>> 
>>>> diff --git a/lib/Target/R600/SIInstrInfo.cpp b/lib/Target/R600/SIInstrInfo.cpp
>>>> index d68858e..2f26861 100644
>>>> --- a/lib/Target/R600/SIInstrInfo.cpp
>>>> +++ b/lib/Target/R600/SIInstrInfo.cpp
>>>> @@ -1358,8 +1358,27 @@ void SIInstrInfo::legalizeOperands(MachineInstr *MI) const {
>>>>  // XXX - Do any VOP3 instructions read VCC?
>>>>  // Legalize VOP3
>>>>  if (isVOP3(MI->getOpcode())) {
>>>> -    int VOP3Idx[3] = {Src0Idx, Src1Idx, Src2Idx};
>>>> +    const MCInstrDesc &Desc = get(MI->getOpcode());
>>>>    unsigned SGPRReg = AMDGPU::NoRegister;
>>>> +
>>>> +    int VOP3Idx[3] = { Src0Idx, Src1Idx, Src2Idx };
>>>> +
>>>> +    // First we need to consider the instruction's operand requirements before
>>>> +    // legalizing. Some operands are required to be SGPRs, but we are still
>>>> +    // bound by the constant bus requirement to only use one.
>>>> +    //
>>>> +    // If the operand's class is an SGPR, we can never move it.
>>>> +    for (unsigned i = 0; i < 3; ++i) {
>>>> +      int Idx = VOP3Idx[i];
>>>> +      if (Idx == -1)
>>>> +        break;
>>>> +
>>>> +      if (RI.isSGPRClassID(Desc.OpInfo[Idx].RegClass)) {
>>>> +        SGPRReg = MI->getOperand(Idx).getReg();
>>>> +        break;
>>>> +      }
>>>> +    }
>>>> +
>>> 
>>> I think it may be possible to move some of this logic into
>>> isOperandLegal.  You might want to look at patch #3 from here
>>> first:
>>> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140915/235726.html
>> 
>> I’m not sure how. I think isOperandLegal should only consider one operand at a time, and this looks over all of the operands at once. The way isOperandLegal checks all operands of the instruction after looking at one that uses the constant bus is sort of confusing, since it makes it more of a areInstructionOperandsLegal
> 
> isOperandLegal() is currently being used for two types of checks:
> 
> 1. Is this instruction's operand legal?
> 2. If I replace the operand at OpIdx with this new operand, will
> the instruction be legal?
> 
> For these kinds of checks, I can't really see how they
> can be done accurately without considering all the operands,
> mainly because of the constant bus restrictions.
> 
> Anyway, all these patches are fine as is, we can work out the issues
> with isOperandLegal() later.  I tested the patches attached to this email,
> plus patch 1 from your previous email, which wasn't attached here and there
> were no regressions.
> 
> -Tom


r218526 - r218536
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140926/4bcd520d/attachment.html>