[PATCH] [BUG 22755] Fix bugs in CombinToPreIndexedLoadStore

[Hal Finkel]

----- Original Message -----
> From: "Yin Ma" <yinma at codeaurora.org>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: llvm-commits at cs.uiuc.edu
> Sent: Tuesday, March 31, 2015 1:01:22 PM
> Subject: RE: [PATCH] [BUG 22755] Fix bugs in CombinToPreIndexedLoadStore
> 
> Hi Hal,
> 
> Thanks, I will put more comments into it.

If you won't be able to get to this soon, I'll be happy to do it for you. I don't want us to forget about it.

Thanks again,
Hal

> 
> Yin
> 
> -----Original Message-----
> From: Hal Finkel [mailto:hfinkel at anl.gov]
> Sent: Tuesday, March 31, 2015 10:14 AM
> To: Yin Ma
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [PATCH] [BUG 22755] Fix bugs in
> CombinToPreIndexedLoadStore
> 
> Hi Yin,
> 
> I apologize for taking so long to look at this. The patch is fine, my
> only request is that comment you currently add:
> 
> +      // Must come from the same resNo.
> +      if (Op0.getResNo() != Ptr.getOperand(0).getResNo()) {
> +        OtherUses.clear();
> +        break;
> +      }
> 
> could better explain what is going on. In the bug report, you explain
> that this is to prevent picking up an add of the updated index from
> a pre-increment load with the result of the load itself. Could you
> please add text to that effect in the comment. Otherwise, LGTM.
> 
> Thanks again,
> Hal
> 
> ----- Original Message -----
> > From: "Yin Ma" <yinma at codeaurora.org>
> > To: llvm-commits at cs.uiuc.edu
> > Cc: hfinkel at anl.gov
> > Sent: Wednesday, March 11, 2015 12:40:45 PM
> > Subject: [PATCH] [BUG 22755] Fix bugs in
> > CombinToPreIndexedLoadStore
> > 
> > 
> > 
> > 
> > Hi,
> > 
> > 
> > 
> > This simple change is for Bug 22755
> > 
> > https://llvm.org/bugs/show_bug.cgi?id=22755
> > 
> > 
> > 
> > Basically, the code tried to fold two adds. However, in a situation
> > where it is a preindex load L converted before, it is possible that
> > the input of one add is from the result of L and the input of
> > another
> > add is from the updated index of L. These two adds cannot be
> > combined
> > together clearly, so we need check to make sure two adds are from
> > the
> > same ResNo by adding code in the patch.
> > 
> > 
> > 
> > Unfortunately, I don’t have a test case for it. However, it should
> > be
> > quite clear to see the change.
> > 
> > Please review.
> > 
> > 
> > 
> > Thanks,
> > 
> > 
> > 
> > Yin
> 
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
> 
> 

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