[LLVMdev] Preserving the order of WRITE_REGISTER, READ_REGISTER in IR and ISelDAG

[Johnson, Nicholas Paul]

Thanks Hal, and sorry about the confusion between llvm-commits, llvm-dev, and Bugzilla.  For reference, this thread discusses Bug#23389.

From: Hal Finkel [mailto:hfinkel at anl.gov]
>It should not be modeled as read-write, however, but just as a read (thus, I
>think that IntrReadMem is appropriate).
>

Some embedded programmers might prefer the R/W semantics to prevent the compiler from re-ordering explicit register reads, but either semantics are acceptable for my purposes.



>> >  // excerpt from lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
>> >  case Intrinsic::write_register: {
>> >    Value *Reg = I.getArgOperand(0);
>> >    Value *RegValue = I.getArgOperand(1);
>> >    SDValue Chain = getValue(RegValue).getOperand(0);
...
>> >Is this a bug?  It seems that the chain should instead be the
>> >selection DAG
>> >root, as so:
>> >
>> >-    SDValue Chain = getValue(RegValue).getOperand(0);
>> >+   SDValue Chain = DAG.getRoot();
>
>No, I think the existing code is right. The SDAG intrinsic node should take the
>incoming chain as its first argument.
>

I disagree that the expression 'getValue(RegValue).getOperand(0)' evaluates to a chain.

RegValue refers to the second operand of llvm.write_register, i.e., an llvm::Value being assigned to a register.  The SDValue result of getValue(RegValue) references an SDNode that doesn't necessarily have an operand 0, and if it does, that operand is not necessarily a chain.

In particular, consider:
1. if 'RegValue' is an llvm::ConstantInt, and
2. SelectionDAGBuilder::getValue has not yet cached it in NodeMap, 
3. then SelectionDAGBuilder::getValueImpl will try to materialize it via SelectionDAG::getConstant.
4. If that constant is not yet in the SelectionDAG's CSEMap,
5. then SelectionDAG::getConstant returns an SDValue referencing a new ConstantSDNode.
6. That ConstantSDNode has no operands, let alone a chain operand.




There was also a third part of the patch that I mentioned at Bug#23389, but which was not mentioned in my initial llvm-dev post.  I'll mention it here for sake of discussion:

SelectionDAGISel::Select_WRITE_REGISTER transcribes ISD::WRITE_REGISTER nodes into ISD::CopyToReg nodes.  In doing so, it ignore replaces the WRITE_REGISTER's chain operand with CurDAG->getEntryNode().  I think it should instead preserve the chain operand found in the WRITE_REGISTER node.  An analogous fix is necessary for in SelectionDAGISel::Select_READ_REGISTER.



N