This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Implement rem opcode
ClosedPublic

Authored by tbaeder on Sep 27 2022, 8:43 AM.

Details

Summary

Something easier to review I hope :)

Implement an opcode to get the remainder of the divison between LHS and RHS.

Diff Detail

Event Timeline

tbaeder created this revision.Sep 27 2022, 8:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2022, 8:43 AM
tbaeder requested review of this revision.Sep 27 2022, 8:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2022, 8:43 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tbaeder updated this revision to Diff 463245.Sep 27 2022, 8:45 AM
erichkeane added inline comments.Sep 27 2022, 8:51 AM
clang/lib/AST/Interp/Integral.h
213

Its a touch weird we return 'bool' in all these places, rather than an Optional<Integral>, right? Perhaps a refactor idea for a future patch.

clang/lib/AST/Interp/Interp.h
171

I get that we 'know' that rem doesn't have an error case, but it seems like a mistake to not check the return value... Perhaps an even better reason to use optional.

clang/lib/AST/Interp/Opcodes.td
57

Perhaps IntegralTypeClass, unless we expect floats to be here too?

62

What does "Alu" mean here?

tbaeder updated this revision to Diff 463251.Sep 27 2022, 8:58 AM
tbaeder added inline comments.
clang/lib/AST/Interp/Integral.h
213

The really weird part is that false here suddenly means success, but I get your point. Does using Optional introduce any sort of performance penalty if this is being called a lot?

clang/lib/AST/Interp/Opcodes.td
57

Floats don't have to be in there I guess, but so far I'd say yes, they should be, because they are usable everywhere this class is used, afaik.

62

I didn't introduce the name but I assumed it comes from "arithmetic logical unit".

erichkeane accepted this revision.Sep 27 2022, 9:00 AM
erichkeane added inline comments.
clang/lib/AST/Interp/Integral.h
213

'false' as success is actually pretty common in clang, we use that all over the place. I don't believe Optional would have any negative penalty, at least compared to a pointer-param?

clang/lib/AST/Interp/Opcodes.td
57

Ok then, thanks.

62

Ah! That could be. Makes sense.

This revision is now accepted and ready to land.Sep 27 2022, 9:00 AM
tbaeder marked 7 inline comments as done.Sep 27 2022, 9:04 AM
tbaeder added inline comments.
clang/lib/AST/Interp/Integral.h
213

Yeah I know it's being used elsewhere in the codebase like this, but the all the AST visitors in the interpreter code return false for failure, so this is kinda confusing to me.

I'll note the refactoring for later.

shafik added inline comments.Sep 27 2022, 10:04 PM
clang/lib/AST/Interp/Interp.h
164

You also need to catch when the result is not representable e.g INT_MIN % -1

see CheckICE(...)

tbaeder updated this revision to Diff 463456.Sep 28 2022, 1:14 AM
tbaeder marked an inline comment as done.
tbaeder updated this revision to Diff 463467.Sep 28 2022, 2:33 AM
tbaeder added inline comments.Sep 28 2022, 2:34 AM
clang/lib/AST/Interp/Interp.h
173

Resultis unused in here now I guess. Also RHSInt

tbaeder updated this revision to Diff 463472.Sep 28 2022, 2:46 AM
tbaeder marked an inline comment as done.
aaron.ballman added inline comments.Sep 28 2022, 5:01 AM
clang/test/AST/Interp/literals.cpp
161

Same question here as with div in regards to testing float behavior. (If you don't intend to support floats yet, make sure the commit message makes that clear.)

tbaeder added inline comments.Sep 28 2022, 5:10 AM
clang/test/AST/Interp/literals.cpp
161

Ultimately sure, but as of right now, floats aren't supported at all, so nope.

aaron.ballman added inline comments.Sep 28 2022, 5:24 AM
clang/test/AST/Interp/literals.cpp
161

Okay, fine by me.

In terms of the request by @shafik for INT_MIN % -1, keep in mind there are arbitrary width integer types like _BitInt, so you should add test coverage for code like:

constexpr _BitInt(7) Val = -64;
static_assert(Val % (_BitInt(7)-1, "");

and similar for division.

tbaeder added inline comments.Sep 28 2022, 6:25 AM
clang/test/AST/Interp/literals.cpp
161

I just checked, _BitInt doesn't work at all; it bails out when creating the Descriptor for the variable.

I already added the suggested note for the INT_MIN case in this patch, but I wasn't sure how to test is properly. Should I just use a fixed-width integer type and copy its _MIN value in the test?

aaron.ballman added inline comments.Sep 28 2022, 6:54 AM
clang/test/AST/Interp/literals.cpp
161

Ah, well that's something we should fix! :-D

I already added the suggested note for the INT_MIN case in this patch, but I wasn't sure how to test is properly. Should I just use a fixed-width integer type and copy its _MIN value in the test?

You should be able to use:

#define INT_MIN (~__INT_MAX__)

(and similar for the other MIN macros) which relies on the predefined macro instead of something from a header.

tbaeder updated this revision to Diff 463543.Sep 28 2022, 7:02 AM
tbaeder marked an inline comment as done.Sep 28 2022, 7:03 AM
tbaeder added inline comments.
clang/test/AST/Interp/literals.cpp
161

Done, thanks. But I guess this now depends on https://reviews.llvm.org/D134804 since it uses ~ haha

tbaeder marked an inline comment as done.Sep 29 2022, 11:50 PM
tbaeder added inline comments.
clang/lib/AST/Interp/Interp.h
164

I added the check here some lines below; Is that alright? If so I'd add the same code to the div() implementation.

aaron.ballman added inline comments.Sep 30 2022, 5:52 AM
clang/lib/AST/Interp/Interp.h
164

Looks correct to me.

170

I really like how clear and generalized this predicate is!

shafik accepted this revision.Sep 30 2022, 1:43 PM

LGTM

This revision was landed with ongoing or failed builds.Oct 14 2022, 4:01 AM
This revision was automatically updated to reflect the committed changes.