Page MenuHomePhabricator

[RISCV] Support negative constants in CompressInstEmitter
ClosedPublic

Authored by simoncook on Mar 25 2020, 5:22 AM.

Details

Summary

Some compressed instructions match against negative values; store
immediates as a signed value such that these patterns will now match
the intended instructions.

Diff Detail

Event Timeline

simoncook created this revision.Mar 25 2020, 5:22 AM

Context for this patch: it's needed for CompressPats for bitmanip - since one of the compressed instructions should match against -1

asb added a comment.Mar 26 2020, 5:33 AM

I was thinking about making it clearer why this cast is necessary. The first thought was obviously to add a comment to explain (similar to the comment in your review description). But maybe there's an argument for adding a small helper? Although it would literally just be doing a cast, it's name and description should make it much clearer what is going on. I don't feel super strongly about that approach though - so do speak up if you disagree!

simoncook updated this revision to Diff 252826.Mar 26 2020, 6:33 AM

Actually solve the underlying problem.

Contrary to my original description, it is not TableGen that stores immediates as unsigned values, but the OpData structure. We are implicitly casting all immediates from signed to unsigned values. By correcting the type and removing that implicit cast, no explicit cast is needed later on.

simoncook edited the summary of this revision. (Show Details)Mar 26 2020, 6:34 AM
asb accepted this revision.Mar 26 2020, 7:48 AM

This is a much simpler fix! Looks good to me.

This revision is now accepted and ready to land.Mar 26 2020, 7:48 AM
This revision was automatically updated to reflect the committed changes.
apazos added a comment.Apr 8 2020, 6:46 PM

To clarify, we were not missing in compressed instructions before, this change is for the future compressed instructions to be added, confirmed? Otherwise I need to check why we missed it with the fuzzer.

To clarify, we were not missing in compressed instructions before, this change is for the future compressed instructions to be added, confirmed? Otherwise I need to check why we missed it with the fuzzer.

Yes, this only became a thing because one of the bitmanip proposed/wishlist compressed instructions wants to match against -1, all the currently ratified instructions either match immediate 0 or greater, so this wouldn't have come up before.