[LLVMdev] DAGCombiner::ReduceLoadWidth bug?

[Eli Friedman]

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