Page MenuHomePhabricator

[VectorUtils] Don't try and truncate PHIs to a smaller bitwidth
ClosedPublic

Authored by jmolloy on Mar 24 2016, 6:03 AM.

Details

Reviewers
sbaranga
Summary

We already try not to truncate PHIs in computeMinimalBitwidths. LoopVectorize can't handle it and we really don't need to, because both induction and reduction PHIs are truncated by other means.

However, we weren't bailing out in all the places we should have, and we ended up by returning a PHI to be truncated, which has caused PR27018.

This fixes PR17018.

Diff Detail

Repository
rL LLVM

Event Timeline

jmolloy updated this revision to Diff 51547.Mar 24 2016, 6:03 AM
jmolloy retitled this revision from to [VectorUtils] Don't try and truncate PHIs to a smaller bitwidth.
jmolloy updated this object.
jmolloy added a reviewer: sbaranga.
jmolloy set the repository for this revision to rL LLVM.
jmolloy added a subscriber: llvm-commits.
sbaranga added inline comments.Mar 24 2016, 6:34 AM
lib/Analysis/VectorUtils.cpp
557

Hi James,

Why would this be a problem for the loop vectorizer (we should state this here)? Also, won't indvars generally extend the phi?

Cheers,
Silviu

Hi Silviu,

Primarily because the LV just doesn’t support PHIs and teaching it would be quite some work. We already have a comment above saying we don’t support PHIs; we just weren’t catching all of them at that point!

Indvars is one of the reasons we don’t support PHI truncation - it’s a minefield. Induction variables often hold pointer values, and calculating the exact situations where these can be truncated successfully was rather tricky when I originally wrote this code.

I could probably put effort into supporting PHIs, but it’s most important to fix this crasher at least in the meantime.

James

sbaranga accepted this revision.Mar 24 2016, 7:24 AM
sbaranga edited edge metadata.

I could probably put effort into supporting PHIs, but it’s most important to fix this crasher at least in the meantime.

I don't think there's any need for this, I was just trying to better understand what was going on.

LGTM!

This revision is now accepted and ready to land.Mar 24 2016, 7:24 AM
jmolloy closed this revision.Apr 27 2016, 7:25 AM