Page MenuHomePhabricator

Make GlobalsAA ignore dead constant expressions.
ClosedPublic

Authored by efriedma on Aug 31 2016, 3:20 PM.

Details

Summary

Slightly improves the precision of GlobalsAA in certain situations, and makes the behavior of optimization passes more predictable.

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma updated this revision to Diff 69912.Aug 31 2016, 3:20 PM
efriedma retitled this revision from to Make GlobalsAA ignore dead constant expressions..
efriedma updated this object.
efriedma added reviewers: jmolloy, sanjoy, mkuper.
efriedma set the repository for this revision to rL LLVM.
efriedma added a subscriber: llvm-commits.
mkuper added inline comments.Aug 31 2016, 3:56 PM
lib/Analysis/GlobalsModRef.cpp
277

Don't know enough about GlobalsAA to approve this anyway, but as a matter of principle - is it acceptable for an analysis pass to modify the IR?

hfinkel added inline comments.
lib/Analysis/GlobalsModRef.cpp
277

No, they shouldn't.

sanjoy requested changes to this revision.Sep 11 2016, 10:05 PM
sanjoy edited edge metadata.

Marking as requiring changes to get this out of my phabricator queue. I think @mkuper 's comment on not modifying IR from analyses is a blocker.

This revision now requires changes to proceed.Sep 11 2016, 10:05 PM
efriedma updated this revision to Diff 71896.Sep 19 2016, 5:44 PM
efriedma edited edge metadata.

Revised approach which doesn't modify the IR.

efriedma edited edge metadata.Sep 22 2016, 6:03 PM
efriedma added a subscriber: chandlerc.

Ping.

Random drop-by comment.

test/Analysis/GlobalsModRef/dead-uses.ll
38

There is no #0 in the file.

efriedma updated this revision to Diff 73107.Sep 30 2016, 11:45 AM

Updated test.

sanjoy added inline comments.Sep 30 2016, 12:09 PM
lib/Analysis/GlobalsModRef.cpp
369

How about pulling this out of the loop? That is, do:

if (auto *C = dyn_cast<Constant>(V))
  if (!C->isConstantUsed())
    return false;
test/Analysis/GlobalsModRef/dead-uses.ll
12

Some more context here will be helpful -- are you trying to check if the load was hoisted out of the loop? If so, then a CHECK: entry: before the load will be nice (since there are two instances of br label %for.inc).

efriedma added inline comments.Sep 30 2016, 1:04 PM
lib/Analysis/GlobalsModRef.cpp
369

That wouldn't work... say V is the global @a (with live loads and stores which access it), and I is a constant expression ptrtoint @a to i32. We want to ignore I if it's dead; checking V is useless.

test/Analysis/GlobalsModRef/dead-uses.ll
12

Yes; I'll change this to make that more clear.

efriedma updated this revision to Diff 73127.Sep 30 2016, 1:27 PM

Make test more clear.

sanjoy accepted this revision.Sep 30 2016, 4:22 PM
sanjoy edited edge metadata.

I don't understand GlobalsModRef deeply, but this seems obvious so lgtm.

This revision is now accepted and ready to land.Sep 30 2016, 4:22 PM
chandlerc accepted this revision.Sep 30 2016, 11:42 PM
chandlerc added a reviewer: chandlerc.

Thanks Sanjoy for doing the review. FWIW, I agree, LGTM.

efriedma closed this revision.Oct 3 2016, 5:13 PM
efriedma marked 7 inline comments as done.

Merged in r283165.