[PATCH] D47475: [Local] Make DoesKMove required for combineMetadata.

[Florian Hahn via Phabricator via llvm-commits]

fhahn updated this revision to Diff 156287.
fhahn retitled this revision from "[Local] Make KDominatesJ required for combineMetadata." to "[Local] Make DoesKMove required for combineMetadata.".
fhahn edited the summary of this revision.
fhahn added a comment.

In https://reviews.llvm.org/D47475#1167326, @efriedma wrote:

> Given that it's UB to violate nonnull metadata, the metadata on J doesn't actually matter; the important question is whether K moves.  If it does, we have to strip the nonnull metadata; it it doesn't, we don't.  So "KDominatesJ" shouldn't exist, as far as I can tell; the boolean should just be "DoesKMove".


Hm I realize "KDominatesJ" is not too clear, as we could hoist an instruction before running combineMetadata. It was meant to mean "in the original IR, K dominated J", so before any moving/hoisting. "DoesKMove" sounds slightly clearer.  https://reviews.llvm.org/D47339 only uses the fact to not drop K's metadata if K does not move, as this enables keeping nonnull in more valid cases.

If combineMetadata is used to combine the metadata from l1 and l2 (which would preserve !nonnull, as both loads have it) and l1 gets hoisted, that would be illegal IIUC. However it looks like when we do hoisting, we create a new load without metadata, so this should not be a problem in existing code. Also it allows sinking a load and preserving the nonnull metadata, if there are loads in all predecessors and they all have nonnull.

    br i1 %x, label %bb2, label %bb2
  
  bb1:
    ....
    %l1 = load .... !nonnull !0
    ....
  
  bb2:
    ...
    %l2 = load .... !nonnull !0

What do you think?


https://reviews.llvm.org/D47475

Files:
  include/llvm/Transforms/Utils/Local.h
  lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
  lib/Transforms/InstCombine/InstCombinePHI.cpp
  lib/Transforms/Scalar/GVNHoist.cpp
  lib/Transforms/Scalar/GVNSink.cpp
  lib/Transforms/Scalar/JumpThreading.cpp
  lib/Transforms/Scalar/MemCpyOptimizer.cpp
  lib/Transforms/Utils/Local.cpp
  lib/Transforms/Utils/SimplifyCFG.cpp
  test/Transforms/GVNHoist/hoist-md.ll
  test/Transforms/GVNSink/sink-combine-metadata.ll
  test/Transforms/InstCombine/load-combine-metadata-dominance.ll
  test/Transforms/InstCombine/phi-load-metadata-dominance.ll
  test/Transforms/JumpThreading/combine-metadata.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D47475.156287.patch
Type: text/x-patch
Size: 14587 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180719/64469494/attachment.bin>