<div dir="ltr">Hi Roman,<div><br></div><div>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.</div><div><br></div><div>Cheers,</div><div><br></div><div>James</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, 31 Aug 2019 at 11:51, Roman Lebedev <<a href="mailto:lebedev.ri@gmail.com">lebedev.ri@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Test missing.<br>
<br>
On Sat, Aug 31, 2019 at 1:44 PM James Molloy via llvm-commits<br>
<<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
><br>
> Author: jamesm<br>
> Date: Sat Aug 31 03:46:16 2019<br>
> New Revision: 370576<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=370576&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=370576&view=rev</a><br>
> Log:<br>
> [DAGCombiner] Don't create illegal narrow stores<br>
><br>
> Narrowing stores when the target doesn't support the narrow version<br>
> forces the target to expand into a load-modify-store sequence, which<br>
> is highly suboptimal. The information narrowing throws away (legality<br>
> of the inverse transform) is hard to re-analyze. If the target doesn't<br>
> support a store of the narrow type, don't narrow even in pre-legalize<br>
> mode.<br>
><br>
> No test as this is DAGCombiner and depends on target bits.<br>
><br>
> Modified:<br>
>     llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp<br>
><br>
> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=370576&r1=370575&r2=370576&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=370576&r1=370575&r2=370576&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)<br>
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Sat Aug 31 03:46:16 2019<br>
> @@ -14737,10 +14737,15 @@ ShrinkLoadReplaceStoreWithStore(const st<br>
><br>
>    // Check that it is legal on the target to do this.  It is legal if the new<br>
>    // VT we're shrinking to (i8/i16/i32) is legal or we're still before type<br>
> -  // legalization.<br>
> -  MVT VT = MVT::getIntegerVT(NumBytes*8);<br>
> +  // legalization (and the target doesn't explicitly think this is a bad idea).<br>
> +  MVT VT = MVT::getIntegerVT(NumBytes * 8);<br>
> +  const TargetLowering &TLI = DAG.getTargetLoweringInfo();<br>
>    if (!DC->isTypeLegal(VT))<br>
>      return SDValue();<br>
> +  if (St->getMemOperand() &&<br>
> +      !TLI.allowsMemoryAccess(*DAG.getContext(), DAG.getDataLayout(), VT,<br>
> +                              *St->getMemOperand()))<br>
> +    return SDValue();<br>
><br>
>    // Okay, we can do this!  Replace the 'St' store with a store of IVal that is<br>
>    // shifted by ByteShift and truncated down to NumBytes.<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>