llvm dev - Oct 2011 - [LLVMdev] VirtRegRewriter.cpp: LocalRewriter::ProcessUses()

Hi,

I think I've found a bug in this method.

I ran it on an MI which already had two implicit-use operands, and which defined
a register with a subregindex, ie reg::lo16.

For the def-operand, with a subregindex, an implicit-use operand was added with
this code:

VirtUseOps.insert(VirtUseOps.begin(), MI.getNumOperands());
MI.addOperand(MachineOperand::CreateReg(VirtReg,
                                               false,  // isDef
                                               true)); // isImplicit

As, can be seen, it is presumed that this operand is always the last operand,
this is however not the case. It in fact becomes the first of the impl-use
operands.

It might be preferred to change the addOperand(), so as to always insert the
operand as the last element, or one could handle this locally by looking up the
operand number, which unfortunately is not returned by addOperand().

<   for (unsigned i = 0; i  != MI.getNumOperands(); ++i) {
---
>   for (unsigned i = 0, e = MI.getNumOperands(); i != e; ++i) {
1918c1918
<     if (MO.isImplicit()){
---
>     if (MO.isImplicit())
1923,1928d1922
<       bool inserted=false;
<       for(unsigned oi = 0;oi<VirtUseOps.size();oi++)
<     if(VirtUseOps[oi]==i)
<       inserted=true; 
<       if(inserted)
<     continue; //skip: it was inserted by a partial def below
1930d1923
<     }
1941,1951c1934,1937
<       MachineOperand newMO = MachineOperand::CreateReg(VirtReg,
<                             false,  // isDef
<                               true);  // isImplicit
<       MI.addOperand(newMO); 
<       int opNo; //FIX: the opNo should be returned by addOperand()
<       for(unsigned oi=0;oi<MI.getNumOperands();oi++)
<     if(MI.getOperand(oi).isIdenticalTo(newMO)){ 
<       opNo=oi;
<       break;
<     }
<       VirtUseOps.insert(VirtUseOps.begin(), opNo);
---
>       VirtUseOps.insert(VirtUseOps.begin(), MI.getNumOperands());
>       MI.addOperand(MachineOperand::CreateReg(VirtReg,
>                                               false,  // isDef
>                                               true)); // isImplicit
/Jonas Paulsson

 		 	   		  
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20111007/f5fa0941/attachment.html>

On Oct 7, 2011, at 8:14 AM, Jonas Paulsson wrote:
> Hi,
> 
> I think I've found a bug in this method.
> 
> I ran it on an MI which already had two implicit-use operands, and which
defined a register with a subregindex, ie reg::lo16.
Thanks for fixing this.

Please send patches to llvm-commits, and attach a unified diff (diff -u).

Do you have a test case that exposes this bug?

Note that this whole file is going away after we branch for 3.0, and it is only
used by the linear scan register allocator which isn't used by default in
3.0.

/jakob

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20111010/a1274700/attachment.html>

On Oct 7, 2011, at 8:14 AM, Jonas Paulsson wrote:
> Hi,
> 
> I think I've found a bug in this method.
> 
> I ran it on an MI which already had two implicit-use operands, and which
defined a register with a subregindex, ie reg::lo16.
> 
> For the def-operand, with a subregindex, an implicit-use operand was added
with this code:
> 
> VirtUseOps.insert(VirtUseOps.begin(), MI.getNumOperands());
> MI.addOperand(MachineOperand::CreateReg(VirtReg,
>                                                false,  // isDef
>                                                true)); // isImplicit
> 
> As, can be seen, it is presumed that this operand is always the last
operand, this is however not the case. It in fact becomes the first of the
impl-use operands.
> 
> It might be preferred to change the addOperand(), so as to always insert
the operand as the last element, or one could handle this locally by looking up
the operand number, which unfortunately is not returned by addOperand().
Jonas, after looking into this, I am not sure I understand what you mean.

The VirtUseOps vector contains operand indexes.  For weird reasons, it has
indexes of implicit operands first followed by explicit operand indexes at the
end.

MI.addOperand() will always append an implicit register operand to the end of
the instruction, so MI.getNumOperands() is the index of the added operand.

This index is added to the front of VirtUseOps because it corresponds to an
implicit operand.


Are you saying that MI.addOperand() doesn't append the operand to the end?
It really should.

Also note that MI.addOperand() was mostly rewritten in r140744.

/jakob

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20111012/32c57015/attachment.html>

Yes, I'm saying that the implicit-def operand that was added in this case
ended up as #4, out of 6, when the operands list was reallocated in
addOperand().

If addOperand was rewritten, I think it's best not to add my fix for
ProcessUses(), as I wrote earlier.

Jonas

Subject: Re: [LLVMdev] VirtRegRewriter.cpp: LocalRewriter::ProcessUses()
From: stoklund at 2pi.dk
Date: Wed, 12 Oct 2011 09:14:52 -0700
CC: llvmdev at cs.uiuc.edu
To: jnspaulsson at hotmail.com




On Oct 7, 2011, at 8:14 AM, Jonas Paulsson wrote:Hi,

I think I've found a bug in this method.

I ran it on an MI which already had two implicit-use operands, and which defined
a register with a subregindex, ie reg::lo16.

For the def-operand, with a subregindex, an implicit-use operand was added with
this code:

VirtUseOps.insert(VirtUseOps.begin(), MI.getNumOperands());
MI.addOperand(MachineOperand::CreateReg(VirtReg,
                                               false,  // isDef
                                               true)); // isImplicit

As, can be seen, it is presumed that this operand is always the last operand,
this is however not the case. It in fact becomes the first of the impl-use
operands.

It might be preferred to change the addOperand(), so as to always insert the
operand as the last element, or one could handle this locally by looking up the
operand number, which unfortunately is not returned by addOperand().

Jonas, after looking into this, I am not sure I understand what you mean.
The VirtUseOps vector contains operand indexes.  For weird reasons, it has
indexes of implicit operands first followed by explicit operand indexes at the
end.
MI.addOperand() will always append an implicit register operand to the end of
the instruction, so MI.getNumOperands() is the index of the added operand.
This index is added to the front of VirtUseOps because it corresponds to an
implicit operand.

Are you saying that MI.addOperand() doesn't append the operand to the end?
It really should.
Also note that MI.addOperand() was mostly rewritten in r140744.
/jakob
 		 	   		  
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20111013/4921662f/attachment.html>