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

[Ryan Taylor]

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>