[llvm-commits] DAG type Legalizer bug?

[Dan Gohman]

I get that there's a bug somewhere, but the question is whether the bug
is in DisintegrateMERGE_VALUES or SplitRes_MERGE_VALUES. I don't know
if there's a special reason why DisintegrateMERGE_VALUES was written the
way it is, but it at least doesn't seem accidental.

svn blame attributes the code in SplitRes_MERGE_VALUES r138877, which was
a patch by Micah. Perhaps he knows more about this.

Dan

On Apr 16, 2012, at 1:31 PM, "Guo, Xiaoyi" <Xiaoyi.Guo at amd.com> wrote:

> Yes, I agree the code does what the comment says it does. But still, as I explained in my original email below, there is a bug if you look at how the returned value is used. To prove my point from another point of view, if you look at the svn history of this change, the comment for the revision doesn't show the intention to change the behavior, however, if you look at the change, it did change the behavior by returning the N node itself instead of returning the operand.
> 
> If you agree with my assessment and my fix, I will update the comment of the routine to agree with the fix.
> 
> Xiaoyi
> 
> -----Original Message-----
> From: Dan Gohman [mailto:gohman at apple.com] 
> Sent: Friday, April 13, 2012 5:04 PM
> To: Evan Cheng
> Cc: Guo, Xiaoyi; llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] DAG type Legalizer bug?
> 
> I'm not deeply familiar with that code, but the current code in DisintegrateMERGE_VALUES does precisely what its doxygen comment says it does, which suggest that it's not written that way by accident.
> 
> Dan
> 
> On Apr 13, 2012, at 12:05 PM, Evan Cheng <evan.cheng at apple.com> wrote:
> 
>> Dan, can you review the patch?
>> 
>> Thanks,
>> 
>> Evan
>> 
>> On Apr 13, 2012, at 10:58 AM, "Guo, Xiaoyi" <Xiaoyi.Guo at amd.com> wrote:
>> 
>>> No, this has not been reviewed. I attached the patched again.
>>> 
>>> Thanks,
>>> Xiaoyi
>>> 
>>> From: Evan Cheng [mailto:evan.cheng at apple.com]
>>> Sent: Thursday, April 12, 2012 10:27 PM
>>> To: Guo, Xiaoyi
>>> Cc: llvm-commits at cs.uiuc.edu
>>> Subject: Re: [llvm-commits] DAG type Legalizer bug?
>>> 
>>> Has this been reviewed? Can you post the patch again?
>>> 
>>> Evan
>>> 
>>> On Apr 5, 2012, at 2:23 PM, "Guo, Xiaoyi" <Xiaoyi.Guo at amd.com> wrote:
>>> 
>>> Ping?
>>> 
>>> From: llvm-commits-bounces at cs.uiuc.edu 
>>> [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf OfGuo, Xiaoyi
>>> Sent: Tuesday, April 03, 2012 11:39 AM
>>> To: llvm-commits at cs.uiuc.edu
>>> Subject: [llvm-commits] FW: DAG type Legalizer bug?
>>> 
>>> Please review the attached patch which fixes a bug as described below. Our test case fails on amdil backend. I failed to create a test case with one of the backends built-in to llvm. So I couldn't add a test case to the unit test suite.
>>> 
>>> Thanks,
>>> Xiaoyi
>>> 
>>> From: llvmdev-bounces at cs.uiuc.edu 
>>> [mailto:llvmdev-bounces at cs.uiuc.edu] On Behalf Of Guo, Xiaoyi
>>> Sent: Friday, March 23, 2012 8:11 PM
>>> To: LLVM Developers Mailing List
>>> Subject: [LLVMdev] DAG type Legalizer bug?
>>> 
>>> The following looks like a bug in the legalizer to me.
>>> 
>>> DAGTypeLegalizer::SplitRes_MERGE_VALUES(SDNode*N, unsigned ResNo, SDValue& Lo, SDValue& Hi) {
>>>  SDValue Op = DisintegrateMERGE_VALUES(N, ResNo);
>>>  GetSplitOp(Op, Lo, Hi);
>>> }
>>> 
>>> DisintegrateMERGE_VALUE() returns SDValue(N, ResNo), where N is the MERGE_VALUE node itself.
>>> Then GetSplitOp() tries to retrieve split result for N from the SplitVectors cache and hit assert because split result is for N is not in the cache yet.
>>> 
>>> Seems to me that DisintegrateMERGE_VALUES() should return the corresponding operand for the given ResNo, not the defined value.
>>> 
>>> Please confirm if it's a bug, or if I'm missing something.
>>> 
>>> Thanks,
>>> Xiaoyi
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>> <LegalizeTypes.patch>
>> 
> 
> 
>