This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ] Make sure not to call getZExtValue on a >64 bit constant.
ClosedPublic

Authored by jonpa on Sep 22 2020, 3:19 AM.

Details

Reviewers
uweigand
RKSimon
Summary

Add checks for the bit width in two places in SystemZTargetTransformInfo.cpp

Diff Detail

Event Timeline

jonpa created this revision.Sep 22 2020, 3:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2020, 3:19 AM
jonpa requested review of this revision.Sep 22 2020, 3:19 AM
RKSimon added a subscriber: RKSimon.

test case?

llvm/lib/Target/SystemZ/SystemZTargetTransformInfo.cpp
863

Bit tricky without context diff but I think you can just use C->isZero() and drop the ScalarBits limit

1023

You should be able to do this with CI->getValue().isIntN(16) - no need for getBitWidth() checks

jonpa updated this revision to Diff 293683.Sep 23 2020, 3:19 AM
jonpa marked 2 inline comments as done.

Thanks for review!

Patch updated per review with new test cases.

RKSimon accepted this revision.Sep 23 2020, 4:17 AM

LGTM with one minor

llvm/test/Analysis/CostModel/SystemZ/huge-immediates.ll
1

Might be worth adding a FileCheck - you can use update_analyze_test_checks.py to handle it.

This revision is now accepted and ready to land.Sep 23 2020, 4:17 AM
jonpa added inline comments.Sep 23 2020, 6:15 AM
llvm/test/Analysis/CostModel/SystemZ/huge-immediates.ll
1

The reason that I left that out is that the cost values for these i72 instructions do not quite make sense. A comparison with an i72 immediate gets the cost of '1', which is certainly too little.

However, we do not care about those costs, so it makes most sense to me then to not check for their values either.

Is it then ok to leave it as is, or is it better for some reason to auto-generate the full test with the CHECKs?

RKSimon added inline comments.Sep 23 2020, 6:18 AM
llvm/test/Analysis/CostModel/SystemZ/huge-immediates.ll
1

yes its ok to leave them out if you don't think they're useful - sometimes you learn something "interesting" by including them (especially for non-legal types)

jonpa closed this revision.Sep 23 2020, 6:39 AM

Committed as 370a8c8.