This is an archive of the discontinued LLVM Phabricator instance.

[TTI] Fix getGEPCost() for geps with a single operand
ClosedPublic

Authored by davide on Aug 29 2017, 2:37 PM.

Details

Summary

Previously this would sporadically crash as TargetType was never initialized.
I just mimiced what the basic model does here, marking it as free. I'm happy to change this to Basic if folks think it's more appropriate.

Diff Detail

Repository
rL LLVM

Event Timeline

davide created this revision.Aug 29 2017, 2:37 PM
davide added inline comments.
include/llvm/Analysis/TargetTransformInfoImpl.h
677 ↗(On Diff #113159)

Note, this is not strictly part of the fix (but it's needed to trigger it deterministically, as on my machine the uninitialized pointer get weird values).

fhahn accepted this revision.Aug 31 2017, 12:06 PM

LGTM. Thanks for addressing this issue, this is definitely an improvement and much better than passing an uninitialized value to isLegalAddressingMode.

I think to mirror the behavior of isLegalAddressingMode in this file as closely as possible, we should return TTI::TCC_Free, if !BaseGV, else TTI::TCC_Basic.

The other minor nit is just about the position of the early exit, and I think either position is fine.

include/llvm/Analysis/TargetTransformInfoImpl.h
706 ↗(On Diff #113159)

If I understand correctly, we use this early exit to cover the case where TargetType is NULL, because we didn't execute any iterations of the loop above.

I think it would be slightly clearer to just move the early exit before TargetType, with a comment that this covers the case where TargetType would be null.

This revision is now accepted and ready to land.Aug 31 2017, 12:06 PM
This revision was automatically updated to reflect the committed changes.