[LLVMbugs] [Bug 7992] New: illegal operations introduced by DAGCombiner::ReduceLoadWidth

[bugzilla-daemon at llvm.org]

http://llvm.org/bugs/show_bug.cgi?id=7992

           Summary: illegal operations introduced by
                    DAGCombiner::ReduceLoadWidth
           Product: new-bugs
           Version: trunk
          Platform: Macintosh
        OS/Version: MacOS X
            Status: NEW
          Severity: normal
          Priority: P
         Component: new bugs
        AssignedTo: unassignedbugs at nondot.org
        ReportedBy: jpbonn-keyword-llvmbug.a51747 at corniceresearch.com
                CC: llvmbugs at cs.uiuc.edu


Below is the email exchange describing the problem.  This is present in the SVN
head as of 2010-08-25.

In looking at isLoadExtLegal in TargetLowering.h there are number of other
similar functions (e.g. isIndexedLoadLegal, isTruncStoreLegal,
isIndexedStoreLegal) that could potentially have similar issues.

On Mon, Jul 19, 2010 at 5:21 PM, JP Bonn <jpbonn at corniceresearch.com> wrote:
> > On 7/19/10 10:36 AM, Duncan Sands wrote:
>> >> Hi JP,
>> >>
>> >>
>>> >>> DAGCombiner::ReduceLoadWidth() does the following:
>>> >>> /// ReduceLoadWidth - If the result of a wider load is shifted to right of N
>>> >>> /// bits and then truncated to a narrower type and where N is a multiple
>>> >>> /// of number of bits of the narrower type, transform it to a narrower load
>>> >>> /// from address + N / num of bits of new type. If the result is to be
>>> >>> /// extended, also fold the extension to form a extending load.
>>> >>>
>>> >>> The problem I'm running into is our loads are custom lowered.  Our
>>> >>> architecture does not support loads smaller than 32 bits so we change
>>> >>> these loads into 32 bit loads.  Unfortunately ReduceLoadWidth() then
>>> >>> lowers them back into 16 bit loads.
>>> >>>
>>> >>> Is this a bug in ReduceLoadWidth() or is there something I'm missing?
>>> >>>
>> >> I think it's a bug.  When running after type legalization, the DAG combiner
>> >> should not introduce any illegal types.  When running after operation
>> >> legalization, the DAG combiner should not introduce any illegal operations.
>> >> Consider this code from ReduceLoadWidth:
>> >>
>> >>       if (LegalOperations&&  !TLI.isLoadExtLegal(ISD::SEXTLOAD, ExtVT))
>> >>         return SDValue();
>> >>
>> >> Here you see that it checks whether it is only allowed to produce legal
>> >> operations, and bails out if it would create an illegal extending load.
>> >> However the later logic in ReduceLoadWidth doesn't do any such checking
>> >> as far as I can see, which is wrong.
>> >>
>> >>
> > It does appear the same test should be applied to the ZEXTLOAD case too.
> >
> > For my case where I do custom lowerings TLI.isLoadExtLegal() considers
> > custom lowerings as being legal  Should these tests exclude custom
> > lowerings? TLI.isLoadExtLegal() is not declared as being virtual -
> > should it be?
Post-legalization, operations with custom lowerings shouldn't be
generated by the DAGCombiner; isLoadExtLegal should probably take a
"IsAfterLegalization" bool as an argument or something like that.

-Eli

-- 
Configure bugmail: http://llvm.org/bugs/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.