llvm dev - Mar 2015 - [LLVMdev] ReduceLoadWidth, DAGCombiner and non 8bit loads/extloads question.

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

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, but that's
the design choice made by LLVM: to allow illegal operations AND types
during the pre-legalize phase, 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?

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150304/4dddf6c3/attachment.html>

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
>
>