[PATCH] D73342: Fix EarlyCSE to intersect aliasing metadata.

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/EarlyCSE.cpp:1100
+          if (auto OpInst = dyn_cast<Instruction>(Op)) {
+            combineMetadataForCSE(OpInst, Inst, true);
+          }
----------------
sheredom wrote:
> hfinkel wrote:
> > Why is DoesKMove true here? OpInst dominated Inst in the original IR, no?
> My understanding (albeit naive) of combineMetadataForCSE comes from the fact that at the definition the parameter is called KDominatesJ - which for the load case K does dominate J (whereas for the store case it does not, so I set it false there).
> 
> The fact the declaration and definition have different terms for the same boolean seems troublesome now I think about it.
> The fact the declaration and definition have different terms for the same boolean seems troublesome now I think about it.

Yes, and I'm confused by the naming here. The comments in llvm/include/llvm/Transforms/Utils/Local.h say, "Some metadata can only be kept if K dominates J. For this to be correct, K cannot be hoisted." The comment on combineMetadata likewise says, "Some metadata kinds can only be kept if K does not move, meaning it dominated J in the original IR." The parameter is named DoesKMove, implying that when false, K did not originally dominate J. However, in Local.cpp, the parameter to combineMetadataForCSE is named KDominatesJ. This seems backward (i.e., DoesKMove seems like it should be false if K dominates J). In any case, the implementation of llvm::combineMetadata seems to do the more conservative thing when DoesKMove is true (when DoesKMove is false, it will prefer the metadata from K in some cases).

Thus, I think that it should be false here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73342/new/

https://reviews.llvm.org/D73342