llvm dev - Jul 2010 - [LLVMdev] DAGCombiner::ReduceLoadWidth bug?

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?

Alternatively, I guess I could create target specific nodes for the 
lowered loads.

thanks,
JP

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.

Ciao,

Duncan.
>
> Alternatively, I guess I could create target specific nodes for the
> lowered loads.

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?

If I override TLI.isLoadExtLegal() and exclude custom lowerings it fixes 
my problem but is that really the correct solution?  Should I just use a 
target specific node for my custom lowered loads.