[PATCH] D50854: [LICM] Add a diagnostic analysis for identifying alias information of loads within loop

[Anna Thomas via Phabricator via llvm-commits]

anna added a comment.

In https://reviews.llvm.org/D50854#1202845, @reames wrote:

> Detailed comments inline.
>
> A couple of testing notes:
>
> 1. don't the guard tests change output as well?  Or has Max's fix for that already landed?


I'd tested to see if changing the default value in this patch caused any other LICM test to fail, none did other than the invariant.start case. So, I guess Max's fix has landed for guards.

> 2. we need some tests for readonly calls which are not guards or invariant.starts.

will do.



================
Comment at: lib/Transforms/Scalar/LICM.cpp:1601
+
+// The alias set mechanism used by LICM has a major weakness in that
+// it combines all things which may alias into a single set *before* asking
----------------
reames wrote:
> Comment doesn't seem consistent with this being only a diagnostic mechanism.  I think you need to adjust either your submission comment or this comment to be consistent.  
> 
> (For those reading along, this is a port of a downstream patch.  The primary *upstream* value is for finding AST deficiencies, but we do run with a small non-zero value for this.)
Last part of the comment talks about the complexity, which is the reason for having it as a diagnostic tool (lines 1610 to 1612). I'll add the "diagnostic mechanism" phrase at the start of this comment as well.


Repository:
  rL LLVM

https://reviews.llvm.org/D50854