Page MenuHomePhabricator

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

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



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

Diff Detail

Event Timeline

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

test case?


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


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

jonpa updated this revision to Diff 293683.Wed, Sep 23, 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.Wed, Sep 23, 4:17 AM

LGTM with one minor


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

This revision is now accepted and ready to land.Wed, Sep 23, 4:17 AM
jonpa added inline comments.Wed, Sep 23, 6:15 AM

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.Wed, Sep 23, 6:18 AM

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.Wed, Sep 23, 6:39 AM

Committed as 370a8c8.