<div dir="ltr"><div dir="ltr"><div dir="ltr">Ah, so some transform replaces a shift operand which produces a bogus shift, but that shift isn't visited before the xor from:</div><div dir="ltr"><a href="https://reviews.llvm.org/rL347478">https://reviews.llvm.org/rL347478</a></div><div>...then we still have crash potential. So it's like the relationship between instsimplify and instcombine. Unless we guarantee that we've run simplifications before the more general combines, we have to be aware of bogus instructions. And since we don't run DAGCombiner to fixpoint, there's actually optimization potential from lifting all 'DAG simplifies' to some common place and running that before putting nodes on the worklist.<br></div><div><br></div><div>As a first experiment, I put a 'simplifyShift' call into DAGCombiner::AddUsersToWorklist(), and I get 4 codegen test failures:</div><div>Failing Tests (4):<br>    LLVM :: CodeGen/AMDGPU/load-constant-i16.ll<br>    LLVM :: CodeGen/AMDGPU/load-global-i16.ll<br>    LLVM :: CodeGen/X86/pr32588.ll<br>    LLVM :: CodeGen/X86/pr34855.ll<br><br></div><div>I didn't look at the AMDGPU diffs. For x86, we have what appears to be 1 improvement and 1 neutral diff.<br></div><div><br></div></div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, Nov 26, 2018 at 5:08 PM Friedman, Eli <<a href="mailto:efriedma@codeaurora.org" target="_blank">efriedma@codeaurora.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 11/23/2018 12:05 PM, Sanjay Patel via llvm-commits wrote:<br>
> Author: spatel<br>
> Date: Fri Nov 23 12:05:12 2018<br>
> New Revision: 347502<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=347502&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=347502&view=rev</a><br>
> Log:<br>
> [DAG] consolidate shift simplifications<br>
><br>
> ...and use them to avoid creating obviously undef values as<br>
> discussed in the post-commit thread for r347478.<br>
<br>
This is fine as far as it goes, I guess, but it doesn't really solve <br>
your problem: replaceAllUsesWith and friends still don't call simplifyShift.<br>
<br>
-Eli<br>
<br>
-- <br>
Employee of Qualcomm Innovation Center, Inc.<br>
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project<br>
<br>
</blockquote></div>