This is an archive of the discontinued LLVM Phabricator instance.

[llgo] irgen: fix canAvoid*
ClosedPublic

Authored by axw on Dec 16 2014, 1:32 AM.

Details

Summary

canAvoidElementLoad and canAvoidLoad were incorrectly
eliding loads when an index expression is used as an
another array index expression. This led to a panic.

See comments on https://github.com/go-llvm/llgo/issues/175

Diff Detail

Event Timeline

axw updated this revision to Diff 17322.Dec 16 2014, 1:32 AM
axw retitled this revision from to [llgo] irgen: fix canAvoid*.
axw updated this object.
axw edited the test plan for this revision. (Show Details)
axw added a reviewer: pcc.
axw added a subscriber: Unknown Object (MLST).
pcc added inline comments.Dec 16 2014, 11:39 AM
irgen/ssa.go
710

Can you leave the // ok comment here? This would make it a little clearer that we intend to accept the referrer rather than falling through.

712

(ignore this, phabricator is refusing to delete this comment)

719

If we make this hard-coded constant something like 2*sizeof(int), we can probably be guaranteed that we will never need to check for indexes below.

741

Likewise here.

743

Note that (as far as I can tell) this comparison would always fail because instr at this point cannot be an appropriate size to be used as an index. We might want to panic in the body of this if statement to make it clear what our assumptions are, or just revert this part.

axw updated this revision to Diff 17376.Dec 16 2014, 5:38 PM
axw added inline comments.
irgen/ssa.go
710

ok

719

Good point, array indices are always converted to int. Done.

741

Done.

743

I've changed it to panic, and added a comment in case someone makes the same mistake I did in the future.

pcc accepted this revision.Dec 16 2014, 7:15 PM
pcc edited edge metadata.

LGTM

Do you have commit access?

This revision is now accepted and ready to land.Dec 16 2014, 7:15 PM
axw added a comment.Dec 16 2014, 11:51 PM
In D6676#102307, @pcc wrote:

LGTM

Do you have commit access?

Thanks. No, I don't.

pcc added a comment.Dec 17 2014, 1:45 AM

Please do ask for it (http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access). In the meantime, I've committed this for you.

This revision was automatically updated to reflect the committed changes.