[llvm] r349186 - [SDAG] Ignore chain operand in REG_SEQUENCE when emitting instructions

[Krzysztof Parzyszek via llvm-commits]

Done in r349391.

-Krzysztof

On 12/17/2018 1:43 PM, Friedman, Eli wrote:
> Oh, that makes sense.  Essentially, you have a pattern that expands an 
> ISD::LOAD into a load followed by a REG_SEQUENCE, and the chain gets 
> attached to all the lowered instructions.  I guess I didn't realize that 
> we keep those chains through the DAG scheduler, even for instructions 
> where they aren't necessary.
> 
> Given the way it currently works this seems like the right solution. 
> Please fix the comment to make it clear where the chain comes form.
> 
> -Eli
> 
> On 12/17/2018 8:16 AM, Krzysztof Parzyszek wrote:
>> I looked into this a bit more...
>>
>> Hexagon has a "combine" instruction that takes two 32-bit values and 
>> produces an even-odd register pair that holds the resulting 64-bit 
>> value. There are variants of it for combinations of register and 
>> immediate input values. The variant that takes two registers is 
>> essentially identical in function to REG_SEQUENCE, but since 
>> REG_SEQUENCE is target-independent, we use it instead of the "combine" 
>> to make the code more transparent to optimizations.
>>
>> We don't have extending loads that produce a 64-bit value, all such 
>> loads produce 32 bits. For zero- and any-extending loads we used to 
>> add a zero word to pad it to 64 bits. For that we used one of the 
>> variants of combine that took 0 as the high value and a register as 
>> the low value. (To clarify: we zero-extended any-ext loads.) The 
>> problem started when I switched from zero-extending any-ext loads to 
>> padding the extra 32 bits with IMPLICIT_DEF. Now, instead of the 
>> combine variant that takes an immediate, we started using the variant 
>> that takes two registers. Except that we didn't really, because we use 
>> REG_SEQUENCE in those cases. As a result, we now had an output pattern 
>> that generated a REG_SEQUENCE from a pattern that had a load in it. 
>> There is code in the matcher generator (DAGISelMatcherGen.cpp, 
>> EmitResultInstructionAsOperand in particular) that copies the 
>> "HasChain" property from the input pattern tree to the output, which 
>> caused the REG_SEQUENCE to have a chain.
>>
>> InstrEmitter automatically strips all chain and glue nodes from the 
>> input SDNodes, and the code that deals with creating MachineInstrs 
>> never sees any of it. However, REG_SEQUENCE is handled separately from 
>> other instructions, which is how the chain managed to sneak into the 
>> code that generated the corresponding machine instruction (and where 
>> the failing assertion was located).
>>
>> I'm not sure if it's possible for REG_SEQUENCE to also get glue 
>> through a similar mechanism. I didn't add code for that, but I suppose 
>> it could be needed.
>>
>> -Krzysztof
>>
>>
>> On 12/14/2018 4:15 PM, Krzysztof Parzyszek via llvm-commits wrote:
>>> I don't know exactly where this happens. I can check that next week.
>>>
>>> -Krzysztof
>>>
>>> On 12/14/2018 4:11 PM, Friedman, Eli wrote:
>>>> On 12/14/2018 12:14 PM, Krzysztof Parzyszek via llvm-commits wrote:
>>>>> Author: kparzysz
>>>>> Date: Fri Dec 14 12:14:12 2018
>>>>> New Revision: 349186
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=349186&view=rev
>>>>> Log:
>>>>> [SDAG] Ignore chain operand in REG_SEQUENCE when emitting instructions
>>>>>
>>>>> Modified:
>>>>>      llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
>>>>>
>>>>> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
>>>>> URL: 
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp?rev=349186&r1=349185&r2=349186&view=diff 
>>>>>
>>>>> ============================================================================== 
>>>>>
>>>>> --- llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp (original)
>>>>> +++ llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp Fri Dec 14 
>>>>> 12:14:12 2018
>>>>> @@ -652,6 +652,10 @@ void InstrEmitter::EmitRegSequence(SDNod
>>>>>     const MCInstrDesc &II = TII->get(TargetOpcode::REG_SEQUENCE);
>>>>>     MachineInstrBuilder MIB = BuildMI(*MF, Node->getDebugLoc(), II, 
>>>>> NewVReg);
>>>>>     unsigned NumOps = Node->getNumOperands();
>>>>> +  // REG_SEQUENCE can "inherit" a chain from a subnode.
>>>>
>>>> This seems weird... where is the chain added to the REG_SEQUENCE? If 
>>>> TableGen pattern matching is somehow adding a chain to a 
>>>> REG_SEQUENCE, we should fix TableGen, I think; even if this 
>>>> workaround works here, it seems like the sort of issue someone will 
>>>> stumble over again with a different instruction.
>>>>
>>>> -Eli
>>>>
>>>
>>>
>>
>>
> 


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation