[llvm] r370576 - [DAGCombiner] Don't create illegal narrow stores

[James Molloy via llvm-commits]

Hi Roman,

The commit message explains why there is no test. DAGCombiner is one of the
hardest parts of LLVM to test (this is why GlobalISel is happening); not
only do we have to create a testcase to convince SDAG to go down this path
(notoriously brittle, many of the tests in this area do not currently test
the codepaths they were initially written to test) but we also have to find
a target that displays the particular behaviour. LLVM supports out of tree
targets, and sometimes finding an in-tree target that displays this
characteristic is hard.

Cheers,

James

On Sat, 31 Aug 2019 at 11:51, Roman Lebedev <lebedev.ri at gmail.com> wrote:

> Test missing.
>
> On Sat, Aug 31, 2019 at 1:44 PM James Molloy via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
> >
> > Author: jamesm
> > Date: Sat Aug 31 03:46:16 2019
> > New Revision: 370576
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=370576&view=rev
> > Log:
> > [DAGCombiner] Don't create illegal narrow stores
> >
> > Narrowing stores when the target doesn't support the narrow version
> > forces the target to expand into a load-modify-store sequence, which
> > is highly suboptimal. The information narrowing throws away (legality
> > of the inverse transform) is hard to re-analyze. If the target doesn't
> > support a store of the narrow type, don't narrow even in pre-legalize
> > mode.
> >
> > No test as this is DAGCombiner and depends on target bits.
> >
> > Modified:
> >     llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> >
> > Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=370576&r1=370575&r2=370576&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)
> > +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Sat Aug 31
> 03:46:16 2019
> > @@ -14737,10 +14737,15 @@ ShrinkLoadReplaceStoreWithStore(const st
> >
> >    // Check that it is legal on the target to do this.  It is legal if
> the new
> >    // VT we're shrinking to (i8/i16/i32) is legal or we're still before
> type
> > -  // legalization.
> > -  MVT VT = MVT::getIntegerVT(NumBytes*8);
> > +  // legalization (and the target doesn't explicitly think this is a
> bad idea).
> > +  MVT VT = MVT::getIntegerVT(NumBytes * 8);
> > +  const TargetLowering &TLI = DAG.getTargetLoweringInfo();
> >    if (!DC->isTypeLegal(VT))
> >      return SDValue();
> > +  if (St->getMemOperand() &&
> > +      !TLI.allowsMemoryAccess(*DAG.getContext(), DAG.getDataLayout(),
> VT,
> > +                              *St->getMemOperand()))
> > +    return SDValue();
> >
> >    // Okay, we can do this!  Replace the 'St' store with a store of IVal
> that is
> >    // shifted by ByteShift and truncated down to NumBytes.
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190831/a0722304/attachment.html>