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

[Friedman, Eli via llvm-commits]

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
>>>
>>
>>
>
>

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project