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

[Hal Finkel]

----- Original Message -----
> From: "Nicholas Paul Johnson" <Nicholas.Paul.Johnson at DEShawResearch.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "llvm-commits" <llvm-commits at cs.uiuc.edu>
> Sent: Wednesday, May 6, 2015 9:43:40 AM
> Subject: RE: [LLVMdev] Preserving the order of WRITE_REGISTER, READ_REGISTER in IR and ISelDAG
> 
> 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.

Are you talking about registers that update their values upon being read? (so reading itself updates the state).

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

Ah, indeed. You're right, and the code looks wrong. Calling getRoot should give the right thing. This change LGTM.

 -Hal

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

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory