Page MenuHomePhabricator

Const propagatin after hitting assume bugfix
ClosedPublic

Authored by Prazek on Aug 19 2015, 3:33 PM.

Diff Detail

Event Timeline

Prazek updated this revision to Diff 32627.Aug 19 2015, 3:33 PM
Prazek retitled this revision from to Const propagatin after hitting assume bugfix.
Prazek updated this object.
Prazek added reviewers: rsmith, nicholas, majnemer.
Prazek added a subscriber: llvm-commits.
vsk added a subscriber: vsk.Aug 19 2015, 4:05 PM

A few inline comments --

include/llvm/Transforms/Utils/Local.h
291

"the given edge" is now a bit vague. Could you update the docstring?

lib/IR/Dominators.cpp
154

IMO this is fine as a comment.

If you think it's a good idea to lift this explanation into the assert, I suggest shortening it a bit.

205

Same comment as above.

Prazek updated this revision to Diff 32746.Aug 20 2015, 2:19 PM
Prazek marked 3 inline comments as done.
Prazek updated this revision to Diff 32747.Aug 20 2015, 2:25 PM
Prazek added a subscriber: dberlin.
Prazek updated this revision to Diff 32785.Aug 20 2015, 7:26 PM
dberlin edited edge metadata.Aug 20 2015, 7:31 PM

I assume you are hitting this in switch statements or something similar. I triggered the same in NewGVN.

Note that the comment in dominates is simply wrong.
I discussed with this Rafael (who added this). The callers can, in fact, do *nothing* different than what the function would have to do, and in fact, would have to duplicate a ton of complicated critical edge logic that Dominates handles. That seems really bad to me.

Nowhere else in LLVM do we *assert* because a very dumb caller may cause a performance problem.
The right fix here is to simply remove the assert, and make sure your caller is smart about not calling this unnecessarily.

(IE cache the value of edge domination in cases like this).

nicholas edited edge metadata.Aug 25 2015, 8:55 PM

This seems to be missing a test?

Hmm, yep, my bad, I forgot to add test to check this bug.

Prazek updated this revision to Diff 33357.Aug 27 2015, 1:55 PM
Prazek edited edge metadata.
nlewycky accepted this revision.Aug 27 2015, 1:58 PM
nlewycky edited edge metadata.

LGTM

This revision is now accepted and ready to land.Aug 27 2015, 1:58 PM
Prazek updated this revision to Diff 33385.Aug 27 2015, 5:59 PM
Prazek edited edge metadata.

Small test change before pushing

Prazek closed this revision.Aug 27 2015, 6:04 PM