[LLVMdev] DAGCombiner::ReduceLoadWidth bug?

[JP Bonn]

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.