This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Implement Div opcode
ClosedPublic

Authored by tbaeder on Sep 27 2022, 9:20 AM.

Details

Diff Detail

Event Timeline

tbaeder created this revision.Sep 27 2022, 9:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2022, 9:20 AM
tbaeder requested review of this revision.Sep 27 2022, 9:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2022, 9:20 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tbaeder added inline comments.Sep 27 2022, 9:21 AM
clang/lib/AST/Interp/Interp.h
200

Could use a helper function here and in Rem to reduce code duplication.

erichkeane accepted this revision.Sep 27 2022, 9:24 AM

Seems good enoguh to me.

This revision is now accepted and ready to land.Sep 27 2022, 9:24 AM
shafik added inline comments.Sep 27 2022, 10:07 PM
clang/lib/AST/Interp/Interp.h
194

Just like rem we need to catch INT_MIN / -1

CheckICE(...) looks like it does checking for both in the same spot.

aaron.ballman added inline comments.Sep 28 2022, 4:58 AM
clang/test/AST/Interp/literals.cpp
202

The only test coverage is for integer division; do you intend to also cover floating-point division?

tbaeder marked an inline comment as done.Sep 29 2022, 11:48 PM
tbaeder added inline comments.
clang/test/AST/Interp/literals.cpp
202

I added some float div coverage in https://reviews.llvm.org/D134859

tbaeder marked 2 inline comments as done.Oct 4 2022, 9:47 PM
shafik added inline comments.Oct 5 2022, 10:47 AM
clang/test/AST/Interp/literals.cpp
192

Can we add a test case for INT_MIN / -1 same for %

tbaeder updated this revision to Diff 465667.Oct 6 2022, 12:09 AM
tbaeder marked an inline comment as done.
aaron.ballman accepted this revision.Oct 6 2022, 7:22 AM

LGTM aside from a simple refactoring (feel free to apply it when landing or do it post-commit as an NFC change).

clang/lib/AST/Interp/Interp.h
203–211

We should factor this out into a helper function so we don't have duplication between Div and Rem. Same for the above block checking for division by zero.

LGTM aside from a simple refactoring (feel free to apply it when landing or do it post-commit as an NFC change).

I actually suggested something like this review :P I'll write it down and provide a follow-up NFC commit, thanks.

shafik accepted this revision.Oct 6 2022, 8:52 AM
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.