[LLVMdev] DAGCombiner::ReduceLoadWidth bug?
eli.friedman at gmail.com
Mon Jul 19 23:38:59 PDT 2010
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.
More information about the llvm-dev