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

I'm curious about this code in ReduceLoadWidth (and in DAGCombiner in
general):

if (LegalOperations && !TLI.isLoadExtLegal(ExtType, ExtVT))
   return SDValue
<http://llvm.org/docs/doxygen/html/classllvm_1_1SDValue.html>();

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

Thanks.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150303/48eea60b/attachment.html>

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
>

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>

Ahmed,

  It looks like shouldReduceLoadWidth is the best solution, since we can
create override and just return false if the new size is 8, this should
avoid generating LD1 in this case. I'm also seeing LD1 being generated by
other opt functions in DAGCombiner, so I'm not sure this is a full solution
for us, unless they are also generating LD1 via ReduceLoadWidth, I'm not
sure yet.

  Though we are not yet using the version of LLVM that has the
shouldReduceLoadWidth virtual function.

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/edd2d625/attachment.html>