Page MenuHomePhabricator

Const propagation after hitting assume bugfix 2
ClosedPublic

Authored by Prazek on Aug 31 2015, 8:42 PM.

Details

Summary

There was infinite loop because it was trying to change assume(true) into assume(true)
Also added handling when assume(false) appear

Diff Detail

Event Timeline

Prazek updated this revision to Diff 33661.Aug 31 2015, 8:42 PM
Prazek retitled this revision from to Const propagation after hitting assume bugfix 2.
Prazek updated this object.
Prazek added reviewers: rsmith, nlewycky, majnemer.
Prazek added a subscriber: llvm-commits.
nlewycky edited edge metadata.Sep 2 2015, 10:15 AM

Typo in summary, "There was ifinite loop" should say "infinite".

include/llvm/IR/Instructions.h
329 ↗(On Diff #33661)

This change isn't logically part of the const propagation bugfix. Either commit it separately or discard it.

lib/Transforms/Scalar/GVN.cpp
1778–1780

GVN can directly modify the CFG (unlike InstCombine which calls setPreservesCFG). Rephrase the comment about unreachable instruction into a TODO.

Prazek updated this revision to Diff 33834.Sep 2 2015, 11:06 AM
Prazek updated this object.
Prazek edited edge metadata.
Prazek marked 2 inline comments as done.
nlewycky accepted this revision.Sep 2 2015, 11:18 AM
nlewycky edited edge metadata.
nlewycky added inline comments.
lib/Transforms/Scalar/GVN.cpp
1779

Typo "cound" -> "could".

This revision is now accepted and ready to land.Sep 2 2015, 11:18 AM

Forgot to write LGTM when I accepted revision.

I heard that accepting revision is stronger that LGTM, so no worries, nobody greps for it (or do they?).

Prazek closed this revision.Sep 2 2015, 5:21 PM
reames added a subscriber: reames.Sep 3 2015, 2:30 PM

FYI - LGTM is the 'official' sign off used by the community. We
frequently take an accepted phabricator revision to have been given an
LGTM, but that's not 'officially' okay. Doesn't really matter in
practice; I'm just being pedantic for the record.