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

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

Thanks for the reply:

So should LLVM continue to assume 8-bit byte addressing? It would be nice,
not only to us but potential future machines, to have a permanent fix to
this assumption? This sounds reasonable yes?

Marking them as Custom in XXXISelLowering still produces error, the
pre-legalize phase is still going to opt to LD1 since it's not caring about
target-specifics. Then, again, we'll be stuck trying to undo the
addressing.

I see what you mean about the DAGCombiner, that's a two part solution,
first turning them into target-specific before any opts in pre-legalize and
then back before any opts in post-legalize. Certainly a potential solution,
though we may lose some optimization opportunities, even on valid loads, as
you mention.

The ultimate issue here, for us, is that LLVM makes the assumption that
every machine is going to support 8-bit byte-addressing. I'm not sure this
is a solid or even reasonable assumption going forward?

Possibly, we could just pseudo support it and expand it later, but the
addressing would still be an issue, I believe.

Thanks.

On Thu, Mar 5, 2015 at 2:09 PM, Ahmed Bougacha <ahmed.bougacha at
gmail.com>
wrote:
> 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
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150305/8f22ee0d/attachment.html>

On Thu, Mar 5, 2015 at 4:19 PM, Ryan Taylor <ryta1203 at gmail.com>
wrote:
> Thanks for the reply:
>
> So should LLVM continue to assume 8-bit byte addressing? It would be nice,
> not only to us but potential future machines, to have a permanent fix to
> this assumption? This sounds reasonable yes?
>
> Marking them as Custom in XXXISelLowering still produces error, the
> pre-legalize phase is still going to opt to LD1 since it's not caring
about
> target-specifics. Then, again, we'll be stuck trying to undo the
addressing.
>
> I see what you mean about the DAGCombiner, that's a two part solution,
first
> turning them into target-specific before any opts in pre-legalize and then
> back before any opts in post-legalize. Certainly a potential solution,
> though we may lose some optimization opportunities, even on valid loads, as
> you mention.
>
> The ultimate issue here, for us, is that LLVM makes the assumption that
> every machine is going to support 8-bit byte-addressing. I'm not sure
this
> is a solid or even reasonable assumption going forward?
Short version: Patches welcome ;)   Long version: in general, if
something is not supported yet, it means no one needed it, so no one
put the work into it.  If you're willing to do so, I'd approve of
fixing the various parts that assume this (if it's only DAGCombines
that's fine).  Then there's the problem of how to test this in-tree
when no in-tree target has these problems (Krzysztof mentions Hexagon,
do you both have the same problem?).  But that's another conversation!
> Possibly, we could just pseudo support it and expand it later, but the
> addressing would still be an issue, I believe.
You might want to look into MachineMemOperands, which tracks the
memory object a load/store points into.  It could help with the
expansion.

-Ahmed