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

[Krzysztof Parzyszek via llvm-commits]

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