[LLVMdev] ReduceLoadWidth, DAGCombiner and non 8bit loads/extloads question.

[Ahmed Bougacha]

On Wed, Mar 4, 2015 at 11:43 AM, Ryan Taylor <ryta1203 at gmail.com> wrote:
> Ahmed,
>
>  Yes, we do not have an 8 bit type and do not support 8 bit loads/extloads.
>
>   For your first post, I imagine that anything that the DAGCombiner does it
> could undo EXCEPT deciding to opt to a type that is not allowed,

No, I think the SelectionDAG legalization should be able to "undo" any
illegal type as well.  For loads/stores, this usually means making the
memory size bigger.  For instance, an i4 store would be legalized to
use i8, and that's what getStoreSize is for.  LLVM assumes 8bit
byte-addressing though, so that's fine.  Again, you can't really
change the number of bytes addressed though, so you can't do much
here.

> but that's
> the design choice made by LLVM: to allow illegal operations AND types during
> the pre-legalize phase, yes

In general, yes.

> So this is either a bug (design flaw) or maybe
> someone more knowledgeable about a way to get around this without having to
> change core code that cannot be put into off. release will chime in. :)
>
>  Yes, via Expand the assert is issued during LegalizeLoadOps (since it's
> trying to legalize the 8 bit extload because the hardware does not support
> it).
>
>  The EXT part is not bugging me, I suppose it's possible that the
> DAGCombiner could optimize to just an 8-bit load, but we've only seen this
> opt if it's going to an extload, this is why I keep mentioning it. 8 bit
> loads (NON_EXTLOAD) is also illegal on our machine. We do not support any
> type of load that is 8 bit. Does LLVM also feel that 8 bit NON_EXTLOAD
> should always be supported also? For example, the Legalize can expand an
> EXTLOAD to NON_EXTLOAD and EXT but that would not help either, since we
> don't support 8 bit loads.
>
>  I would just like to know the cleanest possible solution to this issue
> within LLVM infrastructure, assuming it's possible with the current 'core'
> code?

Well, I can come up with a bunch of hacks to avoid messing with upstream code =)

If you have an answer to the "how do you legalize 8-bit loads?"
problem I mentioned (perhaps MMOs provide enough information), then
the easiest way would be to just do so in your target.  Perhaps a
target-specific DAGCombine would work, or marking loads as "Custom".

Another solution would be to immediately DAGCombine (again,
target-specific code, which IIRC run before target-independent
DAGCombines) loads into target-specific loads.  This means you'd
sacrifice some of the target-independent combines on ISD::LOAD, but
you can avoid that by turning your target-specific XXXISD::LOAD nodes
into target-independent counterparts in another DAGCombine, right
after legalization.

You can also play whack-a-mole with such DAGCombines (there isn't that
much of them on LOAD/STOREs, other operations should be easy to
promote/expand) and submit patches;  I don't think we can justify
keeping them as is if we clearly don't support the result.

-Ahmed

> Thanks.
>
> On Wed, Mar 4, 2015 at 2:20 PM, Ahmed Bougacha <ahmed.bougacha at gmail.com>
> wrote:
>>
>> On Wed, Mar 4, 2015 at 10:49 AM, Ahmed Bougacha
>> <ahmed.bougacha at gmail.com> wrote:
>> > On Wed, Mar 4, 2015 at 10:26 AM, Ryan Taylor <ryta1203 at gmail.com> wrote:
>> >>  Yes, it is breaking during the legalize phase, depending on which
>> >> TargetLowering callback method we use. For example, Custom will let it
>> >> through to instructions selection, which it breaks at the that phase,
>> >> otherwise I believe it breaks during legalization. If we use Expand
>> >> instead,
>> >> the assert during Legalize is: "EXTLOAD should always be supported". I
>> >> don't
>> >> really understand that message :)
>> >
>> > Keep in mind "EXTLOAD" usually means "load, possibly followed by an
>> > extension".  So, the "EXT" part is probably irrelevant here, if that's
>> > what's bugging you ;)
>>
>> Nevermind, grepping around shows this is specifically about
>> ISD::EXTLOAD, in LegalizeLoadOps (LegalizeDAG.cpp).
>>
>> There's some code above, with an "isTypeLegal(SrcVT)" check, that
>> tries to turn an EXTLOAD into LOAD+[SZ]EXT.  I'm guessing that on your
>> target, both the EXTLOAD from i8 and the i8 type are illegal?
>>
>> In that case, again, I don't know how one could legalize this.
>>
>> -Ahmed
>
>