[PATCH] D33179: [Sink] Fix predicate in legality check

[Keno Fischer via llvm-commits]

Ah, I suspect you're right. Every non-intrinsic that's not a LoadInst, but
loads memory, also modifies it, so would be
caught by the earlier check. I think you were right that just changing the
isa to mayReadFromMemory should be sufficient.
For a test, you can use any in-tree intrinsic that reads memory
e.g. `llvm.load.relative`.

On Thu, Jan 11, 2018 at 2:22 PM, <escha at apple.com> wrote:

>
>
> On Jan 11, 2018, at 11:03 AM, Keno Fischer <keno at juliacomputing.com>
> wrote:
>
> I haven't looked at this code in a while, but I think you'll have to
>
> 1) Add an `else if (Inst->mayReadFromMemory()) { return false; }` after
> the check for LoadInst in isSafeToMove as a
>     correctness fix.
>
>
> Is this necessary? I’m fairly sure this should already be covered here:
>
> if (auto CS = CallSite(Inst)) {
>     // Convergent operations cannot be made control-dependent on
> additional
>     // values.
>     if (CS.hasFnAttr(Attribute::Convergent))
>       return false;
>
>     for (Instruction *S : Stores)
>       if (AA.getModRefInfo(S, CS) & MRI_Mod)
>         return false;
>   }
>
> 2) For memory reading instructions you care about add a similar check to
> the one that's there for LoadInst to recover
>     the optimization (if applicable)
> 3) Change the isa<LoadInst> to mayReadFromMemory in IsAcceptableTarget
> 4) Add a test for your regression ;)
>
>
> i’m not sure about the test because this only comes up (for us, at least)
> with out-of-tree intrinsics that access memory (ex: texture reads).
>
> —escha
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180111/d8f9b114/attachment.html>