Page MenuHomePhabricator

[JumpThreading] Change asserts for WantInteger into actual checks
Needs ReviewPublic

Authored by paquette on Apr 2 2021, 1:27 PM.

Details

Summary

After e734e8286b4b521d829aaddb6d1cbbd264953625, it is possible to end up in a situation where an indirectbr is fed by a cast, which is in turn fed by an operation which only produces integers.

indirectbr expects a block address, however these operations can't produce that.

There were several asserts in computeValueKnownInPredecessorsImpl which check that we're not looking for a block address if we're walking through something which can never produce one.

Since it's now possible to hit these asserts, this changes them into actual checks which return false if Preference is not WantInteger.

This adds a testcase which verifies that we don't crash anymore in these situations.

Diff Detail

Event Timeline

paquette created this revision.Apr 2 2021, 1:27 PM
paquette requested review of this revision.Apr 2 2021, 1:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2021, 1:27 PM
nikic added a comment.Apr 5 2021, 2:16 AM

From someone who isn't very familiar with this code: Why do we need the preference flag in the first place? As indirectbr isn't particularly common, can't we check whether the result is a BlockAddress after the fact?

From someone who isn't very familiar with this code: Why do we need the preference flag in the first place? As indirectbr isn't particularly common, can't we check whether the result is a BlockAddress after the fact?

I'm not hugely familiar with this code either, so I can't really say why it exists. It seems like it's mostly only used for getKnownConstant.

I think it might be possible to refactor. It's nice that we can stop the recursive search in computeValueKnownInPredecessorsImpl early by using it though. I'm a little surprised that it wasn't used in that way before.

I don't really have strong opinions either way, WDYT?

aqjune added a comment.Apr 5 2021, 7:18 PM

This tracks down to a commit in 2010: https://github.com/llvm/llvm-project/commit/d9df6eaa9ce20
Interestingly the motivation of introducing ConstantPreference was to properly deal with indirectbr. :)
I have no strong preference either.

Personally, I'd like to avoid refactoring in this patch just to keep things small and avoid any unintended side-effects. Does that sound okay? I think refactoring is probably a good idea in a follow-up. :)