This is an archive of the discontinued LLVM Phabricator instance.

[LICM/AST] Check if the AliasAny set is removed from the tracker.
ClosedPublic

Authored by asbirlea on Sep 10 2019, 4:05 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

asbirlea created this revision.Sep 10 2019, 4:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2019, 4:05 PM

I've no idea if the fix itself makes sense but at least it does seem to fix my reproducer for PR38513 and I didn't see any problems with the patch during the (limited) testing I've done.

Thanks!

test/Transforms/LICM/pr38513.ll
3 ↗(On Diff #219622)

Add a few CHECKs here perhaps?
Right now FileCheck complains there are none:

error: no check strings found with prefix 'CHECK:'
bjope added inline comments.Sep 11 2019, 5:24 AM
lib/Analysis/AliasSetTracker.cpp
707 ↗(On Diff #219622)

Nit: Wouldn't mind if we added a printout here to indicate when we have the AliasAny set (as that was helpful when debugging). I used something like this:

if (AliasAnyAS)
  OS << "Saturated!\n";
test/Transforms/LICM/pr38513.ll
1 ↗(On Diff #219622)

It is recommended to use redirect for the input to opt (https://llvm.org/docs/TestingGuide.html#fragile-tests):

; RUN: opt < %s -enable-mssa-loop-dependency=false -disable-basicaa -alias-set-saturation-threshold=2 -licm -S | FileCheck %s

3 ↗(On Diff #219622)

Yes, CHECK:s are needed. And then "REQUIRES: asserts" can be removed.

asbirlea updated this revision to Diff 219817.Sep 11 2019, 3:22 PM
asbirlea marked 4 inline comments as done.

Address comments.

test/Transforms/LICM/pr38513.ll
3 ↗(On Diff #219622)

The test hits an assertion. It won't trigger without asserts.

This revision is now accepted and ready to land.Sep 12 2019, 10:52 AM
This revision was automatically updated to reflect the committed changes.