Page MenuHomePhabricator

[LV] Fix for PR38110, LV encountered llvm_unreachable()
ClosedPublic

Authored by hsaito on Jul 17 2018, 5:37 PM.

Details

Summary

truncateToMinimalBitWidths() doesn't handle all Instructions and the worst case is compiler crash via llvm_unreachable(). Fix is to add a case to handle PHINode and changed the worst case to NO-OP (from compiler crash).

Diff Detail

Repository
rL LLVM

Event Timeline

hsaito created this revision.Jul 17 2018, 5:37 PM

If I delete IBM target, the LIT test won't crash before the fix. If anyone can suggest an alternative so that I can move the lit test one level up, that would be great.

Seems reasonable to me.

Ideally we should be handling the phi node case (which is why the unreachable was there), but crashing doesn't seem like the right thing to do.

lib/Transforms/Vectorize/LoopVectorize.cpp
3279 ↗(On Diff #155996)

Why do we need to have this if statement if we skip the transformation for unhanded instructions?

Seems reasonable to me.

Ideally we should be handling the phi node case (which is why the unreachable was there), but crashing doesn't seem like the right thing to do.

Personally, I see a value in differentiating explicitly known handling versus just being conservative ----- but I don't mind converting that into a comment instead if that's preferred.

Seems reasonable to me.

Ideally we should be handling the phi node case (which is why the unreachable was there), but crashing doesn't seem like the right thing to do.

Personally, I see a value in differentiating explicitly known handling versus just being conservative ----- but I don't mind converting that into a comment instead if that's preferred.

Sure. I don't have any strong preference here.

LGTM

hsaito accepted this revision.Jul 24 2018, 3:29 PM

Just came back from vacation. Thanks. I take @sbaranga just forgot to set accept the revision and thus taking a liberty of doing so myself.

This revision is now accepted and ready to land.Jul 24 2018, 3:29 PM
This revision was automatically updated to reflect the committed changes.