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

1) It's crashing because LD1 is produced due to LegalOperations=false in
pre-legalize pass. Then Legalization does not know how to handle it so it
asserts on a default case. I don't know if it's a reasonable expectation
or
not but we do not have support for it. I have not tried
overriding shouldReduceLoadWidth.

2) I see, that makes sense to some degree, I'm curious if you can provide
an example? It doesn't seem good to generate something pre-legalize
(target-independent) that you can't then handle when you find that it's
illegal in the very next step that is legalization. I'm guessing, from
what  you and others have said, that it's just expected that every machine
will support 8-bit extloads and if not then the target should handle the
undoing of the target-independent generated 'optimization' via
LegalizeDAG
and TargetLowering callback methods?

3) I have not tried running it on any other target, sorry, I suppose I
should but I didn't want to take the time if I'm not following the right
path here.

Thanks.

We'd love to hear what the community thinks would be the 'best'
solution.
We are trying not to change any 'core' code.



On Tue, Mar 3, 2015 at 2:08 PM, Ahmed Bougacha <ahmed.bougacha at
gmail.com>
wrote:
> On Tue, Mar 3, 2015 at 10:35 AM, Ryan Taylor <ryta1203 at gmail.com>
wrote:
> > I'm curious about this code in ReduceLoadWidth (and in DAGCombiner
in
> > general):
> >
> > if (LegalOperations && !TLI.isLoadExtLegal(ExtType, ExtVT))
> >    return SDValue();
> >
> > LegalOperations is false for the first pre-legalize pass and true for
the
> > post-legalize pass. The first pass is target-independent yes? So that
> makes
> > sense.
> >
> > The issue we are having is this: we don't support 8 bit loads and
we
> don't
> > support 8 bit extloads, so we end up with LD1 with zext after either
the
> > first pass or the second pass (depending on the test case). If we add
the
> > TargetLowering callback method setLoadExtAction(ISD::ZEXTLOAD,
MVT::i8,
> > Expand) then it crashes during legalization
>
> This part is surprising.  What happens?  This seems to me like the
> correct solution.
>
> I'm guessing it's because there's no good way to legalize 8 bit
loads?
>  I have no idea what happens when those are illegal, as I'd expect
> them to be always available?
>
>
> Here's a cheap hack though: I see that ReduceLoadWidth is predicated on
the
> TLI "shouldReduceLoadWidth" hook.  Did you try overriding that to
> avoid creating 8 bit loads?
>
> > and if we don't have that in
> > then it crashes during instruction selection.
> >
> > There are two ways to fix this:
> >
> > 1) Add the setLoadExtAction AND comment out 'LegalOperations
&&' in the
> > conditional. (this solves the problem)
> >
> > 2) Create a custom expand to undo the optimization added by
> ReduceLoadWidth.
> >
> > The 2nd approach seems more in line with what LLVM infrastructure
wants
> but
> > it seems silly to have to undo an optimization?
> >
> > Essentially, we have some bit packing structures and the code is
trying
> to
> > get the upper bits. The initial dag generates an LD2 with srl (which
> makes
> > sense, it's what we want). The DAGCombiner then goes in and
changes that
> LD2
> > with srl to an LD1 zextload, which we don't support.
> >
> > Why is LegalOperations really needed here? What is the purpose and
point
> of
> > this? It seems you could eliminate this and be all the better for it.
>
> FWIW I somewhat agree, and believe this is a common "problem": 
we
> eagerly generate obviously-illegal nodes, because we're before
> legalisation, so it's OK, right?  Except, it's sometimes hard to
> recover from.  Most of the time, it's a good thing, precisely because
> it catches patterns that would be disturbed by legalization.
>
> Did you try running the integration tests - at least X86 - after
> changing the condition?
>
> -Ahmed
>
> > Thanks.
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150303/dc46a4d1/attachment.html>

+Chandler, Hal, Owen, who - among others - know much more than I do.

On Tue, Mar 3, 2015 at 11:57 AM, Ryan Taylor <ryta1203 at gmail.com>
wrote:
> 1) It's crashing because LD1 is produced due to LegalOperations=false
in
> pre-legalize pass. Then Legalization does not know how to handle it so it
> asserts on a default case.
Yes, and where, and on what, does the assert fire?  8-bit load
legalization I assume?
> I don't know if it's a reasonable expectation or
> not but we do not have support for it. I have not tried overriding
> shouldReduceLoadWidth.
>
> 2) I see, that makes sense to some degree, I'm curious if you can
provide an
> example? It doesn't seem good to generate something pre-legalize
> (target-independent) that you can't then handle when you find that
it's
> illegal in the very next step that is legalization.
I believe the general idea is one of separation of concerns:  the way
I see it, the DAGCombiner's job is to canonicalize so that other
(perhaps target-specific) transformations only need to handle the
canonical representation.  Usually, that means optimizing code:  stuff
like e.g. (sub x, x) shouldn't be special-cased in each target (at
least not on ISD nodes ;))
The legalizer (or rather, the various legalizers) later does its own
job, legalizing, after which all code must be "legal".  The further
rounds of DAGCombining just maintain that invariant.

So really, this is a legalization problem:  for instance, is it not
possible for the SelectionDAGBuilder to generate 8-bit loads as well?
In this specific case, I'd rather we avoid the problem altogether, and
would be fine with not doing this kind of transform if the resulting
loads is illegal (and one might argue for the same in all the other
"!LegalOperations" combines).

But the actual solution seems to be with the load legalization:  I
wouldn't be surprised if many other spots assumed 8-bit loads are
legal (as you noticed after disabling ReduceLoadWidth?), and this is
your real problem.

Take this with a grain of salt though, I only ever look at
bog-standard X86-ish targets.  The cc'd people might have a better
argument.

-Ahmed
> I'm guessing, from what
> you and others have said, that it's just expected that every machine
will
> support 8-bit extloads and if not then the target should handle the undoing
> of the target-independent generated 'optimization' via LegalizeDAG
and
> TargetLowering callback methods?
>
> 3) I have not tried running it on any other target, sorry, I suppose I
> should but I didn't want to take the time if I'm not following the
right
> path here.
>
> Thanks.
>
> We'd love to hear what the community thinks would be the 'best'
solution. We
> are trying not to change any 'core' code.
>
>
>
> On Tue, Mar 3, 2015 at 2:08 PM, Ahmed Bougacha <ahmed.bougacha at
gmail.com>
> wrote:
>>
>> On Tue, Mar 3, 2015 at 10:35 AM, Ryan Taylor <ryta1203 at
gmail.com> wrote:
>> > I'm curious about this code in ReduceLoadWidth (and in
DAGCombiner in
>> > general):
>> >
>> > if (LegalOperations && !TLI.isLoadExtLegal(ExtType,
ExtVT))
>> >    return SDValue();
>> >
>> > LegalOperations is false for the first pre-legalize pass and true
for
>> > the
>> > post-legalize pass. The first pass is target-independent yes? So
that
>> > makes
>> > sense.
>> >
>> > The issue we are having is this: we don't support 8 bit loads
and we
>> > don't
>> > support 8 bit extloads, so we end up with LD1 with zext after
either the
>> > first pass or the second pass (depending on the test case). If we
add
>> > the
>> > TargetLowering callback method setLoadExtAction(ISD::ZEXTLOAD,
MVT::i8,
>> > Expand) then it crashes during legalization
>>
>> This part is surprising.  What happens?  This seems to me like the
>> correct solution.
>>
>> I'm guessing it's because there's no good way to legalize 8
bit loads?
>>  I have no idea what happens when those are illegal, as I'd expect
>> them to be always available?
>>
>>
>> Here's a cheap hack though: I see that ReduceLoadWidth is
predicated on
>> the
>> TLI "shouldReduceLoadWidth" hook.  Did you try overriding
that to
>> avoid creating 8 bit loads?
>>
>> > and if we don't have that in
>> > then it crashes during instruction selection.
>> >
>> > There are two ways to fix this:
>> >
>> > 1) Add the setLoadExtAction AND comment out 'LegalOperations
&&' in the
>> > conditional. (this solves the problem)
>> >
>> > 2) Create a custom expand to undo the optimization added by
>> > ReduceLoadWidth.
>> >
>> > The 2nd approach seems more in line with what LLVM infrastructure
wants
>> > but
>> > it seems silly to have to undo an optimization?
>> >
>> > Essentially, we have some bit packing structures and the code is
trying
>> > to
>> > get the upper bits. The initial dag generates an LD2 with srl
(which
>> > makes
>> > sense, it's what we want). The DAGCombiner then goes in and
changes that
>> > LD2
>> > with srl to an LD1 zextload, which we don't support.
>> >
>> > Why is LegalOperations really needed here? What is the purpose and
point
>> > of
>> > this? It seems you could eliminate this and be all the better for
it.
>>
>> FWIW I somewhat agree, and believe this is a common
"problem":  we
>> eagerly generate obviously-illegal nodes, because we're before
>> legalisation, so it's OK, right?  Except, it's sometimes hard
to
>> recover from.  Most of the time, it's a good thing, precisely
because
>> it catches patterns that would be disturbed by legalization.
>>
>> Did you try running the integration tests - at least X86 - after
>> changing the condition?
>>
>> -Ahmed
>>
>> > Thanks.
>> >
>> > _______________________________________________
>> > LLVM Developers mailing list
>> > LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>> >
>
>

Ahmed,

 Yes, this is the case, I'm sure many other 'spots' in DAGCombiner
use this
same check or use a similar check with LegalOperations. It just seems like
bad form to have core code that generates an illegal node that legalization
cannot seem to handle, unless I'm missing something, which is entirely
possible. Potentially we are using the wrong LegalAction, though each I've
tried breaks at different points so I don't think that's it.

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

 The pre-legalization canonicalization makes sense to me but either 1)
legalize should be able to handle everything or 2) nodes that aren't going
to be legal anyways should not be produced (though this obviously
contradicts the term pre-legalize).

  The SelectionDAGBuilder does not generate 8 bit loads nor 8 bit exloads.

 There are other places we could fix this issue, accepting 8 bit extloads
and then expanding it back further down the pipe, but this does seem a bit
hacky.

Thanks.

On Wed, Mar 4, 2015 at 12:53 PM, Ahmed Bougacha <ahmed.bougacha at
gmail.com>
wrote:
> +Chandler, Hal, Owen, who - among others - know much more than I do.
>
> On Tue, Mar 3, 2015 at 11:57 AM, Ryan Taylor <ryta1203 at gmail.com>
wrote:
> > 1) It's crashing because LD1 is produced due to
LegalOperations=false in
> > pre-legalize pass. Then Legalization does not know how to handle it so
it
> > asserts on a default case.
>
> Yes, and where, and on what, does the assert fire?  8-bit load
> legalization I assume?
>
> > I don't know if it's a reasonable expectation or
> > not but we do not have support for it. I have not tried overriding
> > shouldReduceLoadWidth.
> >
> > 2) I see, that makes sense to some degree, I'm curious if you can
> provide an
> > example? It doesn't seem good to generate something pre-legalize
> > (target-independent) that you can't then handle when you find that
it's
> > illegal in the very next step that is legalization.
>
> I believe the general idea is one of separation of concerns:  the way
> I see it, the DAGCombiner's job is to canonicalize so that other
> (perhaps target-specific) transformations only need to handle the
> canonical representation.  Usually, that means optimizing code:  stuff
> like e.g. (sub x, x) shouldn't be special-cased in each target (at
> least not on ISD nodes ;))
> The legalizer (or rather, the various legalizers) later does its own
> job, legalizing, after which all code must be "legal".  The
further
> rounds of DAGCombining just maintain that invariant.
>
> So really, this is a legalization problem:  for instance, is it not
> possible for the SelectionDAGBuilder to generate 8-bit loads as well?
> In this specific case, I'd rather we avoid the problem altogether, and
> would be fine with not doing this kind of transform if the resulting
> loads is illegal (and one might argue for the same in all the other
> "!LegalOperations" combines).
>
> But the actual solution seems to be with the load legalization:  I
> wouldn't be surprised if many other spots assumed 8-bit loads are
> legal (as you noticed after disabling ReduceLoadWidth?), and this is
> your real problem.
>
> Take this with a grain of salt though, I only ever look at
> bog-standard X86-ish targets.  The cc'd people might have a better
> argument.
>
> -Ahmed
>
> > I'm guessing, from what
> > you and others have said, that it's just expected that every
machine will
> > support 8-bit extloads and if not then the target should handle the
> undoing
> > of the target-independent generated 'optimization' via
LegalizeDAG and
> > TargetLowering callback methods?
> >
> > 3) I have not tried running it on any other target, sorry, I suppose I
> > should but I didn't want to take the time if I'm not following
the right
> > path here.
> >
> > Thanks.
> >
> > We'd love to hear what the community thinks would be the
'best'
> solution. We
> > are trying not to change any 'core' code.
> >
> >
> >
> > On Tue, Mar 3, 2015 at 2:08 PM, Ahmed Bougacha <ahmed.bougacha at
gmail.com
> >
> > wrote:
> >>
> >> On Tue, Mar 3, 2015 at 10:35 AM, Ryan Taylor <ryta1203 at
gmail.com>
> wrote:
> >> > I'm curious about this code in ReduceLoadWidth (and in
DAGCombiner in
> >> > general):
> >> >
> >> > if (LegalOperations && !TLI.isLoadExtLegal(ExtType,
ExtVT))
> >> >    return SDValue();
> >> >
> >> > LegalOperations is false for the first pre-legalize pass and
true for
> >> > the
> >> > post-legalize pass. The first pass is target-independent yes?
So that
> >> > makes
> >> > sense.
> >> >
> >> > The issue we are having is this: we don't support 8 bit
loads and we
> >> > don't
> >> > support 8 bit extloads, so we end up with LD1 with zext after
either
> the
> >> > first pass or the second pass (depending on the test case).
If we add
> >> > the
> >> > TargetLowering callback method
setLoadExtAction(ISD::ZEXTLOAD,
> MVT::i8,
> >> > Expand) then it crashes during legalization
> >>
> >> This part is surprising.  What happens?  This seems to me like the
> >> correct solution.
> >>
> >> I'm guessing it's because there's no good way to
legalize 8 bit loads?
> >>  I have no idea what happens when those are illegal, as I'd
expect
> >> them to be always available?
> >>
> >>
> >> Here's a cheap hack though: I see that ReduceLoadWidth is
predicated on
> >> the
> >> TLI "shouldReduceLoadWidth" hook.  Did you try
overriding that to
> >> avoid creating 8 bit loads?
> >>
> >> > and if we don't have that in
> >> > then it crashes during instruction selection.
> >> >
> >> > There are two ways to fix this:
> >> >
> >> > 1) Add the setLoadExtAction AND comment out
'LegalOperations &&' in
> the
> >> > conditional. (this solves the problem)
> >> >
> >> > 2) Create a custom expand to undo the optimization added by
> >> > ReduceLoadWidth.
> >> >
> >> > The 2nd approach seems more in line with what LLVM
infrastructure
> wants
> >> > but
> >> > it seems silly to have to undo an optimization?
> >> >
> >> > Essentially, we have some bit packing structures and the code
is
> trying
> >> > to
> >> > get the upper bits. The initial dag generates an LD2 with srl
(which
> >> > makes
> >> > sense, it's what we want). The DAGCombiner then goes in
and changes
> that
> >> > LD2
> >> > with srl to an LD1 zextload, which we don't support.
> >> >
> >> > Why is LegalOperations really needed here? What is the
purpose and
> point
> >> > of
> >> > this? It seems you could eliminate this and be all the better
for it.
> >>
> >> FWIW I somewhat agree, and believe this is a common
"problem":  we
> >> eagerly generate obviously-illegal nodes, because we're before
> >> legalisation, so it's OK, right?  Except, it's sometimes
hard to
> >> recover from.  Most of the time, it's a good thing, precisely
because
> >> it catches patterns that would be disturbed by legalization.
> >>
> >> Did you try running the integration tests - at least X86 - after
> >> changing the condition?
> >>
> >> -Ahmed
> >>
> >> > Thanks.
> >> >
> >> > _______________________________________________
> >> > LLVM Developers mailing list
> >> > LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150304/0ace34e2/attachment.html>