[PATCH] D134744: [clang][Interp] Implement rem opcode

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc9ad877844a7: [clang][Interp] Implement rem opcode (authored by tbaeder). Changed prior to commit: https://reviews.llvm.org/D134744?vs=463543&id=4

[PATCH] D134744: [clang][Interp] Implement rem opcode

shafik accepted this revision. shafik added a comment. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134744/new/ https://reviews.llvm.org/D134744 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mail

[PATCH] D134744: [clang][Interp] Implement rem opcode

aaron.ballman added inline comments. Comment at: clang/lib/AST/Interp/Interp.h:170 + + if (LHS.isSigned() && LHS.isMin() && RHS.isNegative() && RHS.isMinusOne()) { +APSInt LHSInt = LHS.toAPSInt(); I really like how clear and generalized this predicate is!

[PATCH] D134744: [clang][Interp] Implement rem opcode

tbaeder marked an inline comment as done. tbaeder added inline comments. Comment at: clang/lib/AST/Interp/Interp.h:164 + + if (RHS.isZero()) { +const SourceInfo &Loc = S.Current->getSource(OpPC); shafik wrote: > You also need to catch when the result is not

[PATCH] D134744: [clang][Interp] Implement rem opcode

tbaeder marked an inline comment as done. tbaeder added inline comments. Comment at: clang/test/AST/Interp/literals.cpp:161 +// expected-note {{division by zero}} + + aaron.ballman wrote: > tbaeder wrote: > > aaron.ballman

[PATCH] D134744: [clang][Interp] Implement rem opcode

tbaeder updated this revision to Diff 463543. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134744/new/ https://reviews.llvm.org/D134744 Files: clang/lib/AST/Interp/ByteCodeExprGen.cpp clang/lib/AST/Interp/Integral.h clang/lib/AST/Interp/Interp.h clang/lib/AST/Interp/Opcodes.td

[PATCH] D134744: [clang][Interp] Implement rem opcode

aaron.ballman added inline comments. Comment at: clang/test/AST/Interp/literals.cpp:161 +// expected-note {{division by zero}} + + tbaeder wrote: > aaron.ballman wrote: > > tbaeder wrote: > > > aaron.ballman wrote: > > > >

[PATCH] D134744: [clang][Interp] Implement rem opcode

tbaeder added inline comments. Comment at: clang/test/AST/Interp/literals.cpp:161 +// expected-note {{division by zero}} + + aaron.ballman wrote: > tbaeder wrote: > > aaron.ballman wrote: > > > Same question here as with di

[PATCH] D134744: [clang][Interp] Implement rem opcode

aaron.ballman added inline comments. Comment at: clang/test/AST/Interp/literals.cpp:161 +// expected-note {{division by zero}} + + tbaeder wrote: > aaron.ballman wrote: > > Same question here as with div in regards to testi

[PATCH] D134744: [clang][Interp] Implement rem opcode

tbaeder added inline comments. Comment at: clang/test/AST/Interp/literals.cpp:161 +// expected-note {{division by zero}} + + aaron.ballman wrote: > Same question here as with div in regards to testing float behavior. (If yo

[PATCH] D134744: [clang][Interp] Implement rem opcode

aaron.ballman added inline comments. Comment at: clang/test/AST/Interp/literals.cpp:161 +// expected-note {{division by zero}} + + Same question here as with div in regards to testing float behavior. (If you don't intend t

[PATCH] D134744: [clang][Interp] Implement rem opcode

tbaeder updated this revision to Diff 463472. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134744/new/ https://reviews.llvm.org/D134744 Files: clang/lib/AST/Interp/ByteCodeExprGen.cpp clang/lib/AST/Interp/Integral.h clang/lib/AST/Interp/Interp.h clang/lib/AST/Interp/Opcodes.td

[PATCH] D134744: [clang][Interp] Implement rem opcode

tbaeder added inline comments. Comment at: clang/lib/AST/Interp/Interp.h:173 +APSInt RHSInt = RHS.toAPSInt(); +APSInt Result = LHSInt % RHSInt; + `Result`is unused in here now I guess. Also `RHSInt` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D134744: [clang][Interp] Implement rem opcode

tbaeder updated this revision to Diff 463467. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134744/new/ https://reviews.llvm.org/D134744 Files: clang/lib/AST/Interp/ByteCodeExprGen.cpp clang/lib/AST/Interp/Integral.h clang/lib/AST/Interp/Interp.h clang/lib/AST/Interp/Opcodes.td

[PATCH] D134744: [clang][Interp] Implement rem opcode

tbaeder updated this revision to Diff 463456. tbaeder marked an inline comment as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134744/new/ https://reviews.llvm.org/D134744 Files: clang/lib/AST/Interp/ByteCodeExprGen.cpp clang/lib/AST/Interp/Integral.h clang/lib/AST/Interp/In

[PATCH] D134744: [clang][Interp] Implement rem opcode

shafik added inline comments. Comment at: clang/lib/AST/Interp/Interp.h:164 + + if (RHS.isZero()) { +const SourceInfo &Loc = S.Current->getSource(OpPC); You also need to catch when the result is not representable e.g `INT_MIN % -1` see `CheckICE(...)` CHA

[PATCH] D134744: [clang][Interp] Implement rem opcode

tbaeder marked 7 inline comments as done. tbaeder added inline comments. Comment at: clang/lib/AST/Interp/Integral.h:213 + static bool rem(Integral A, Integral B, unsigned OpBits, Integral *R) { +*R = Integral(A.V % B.V); erichkeane wrote: > tbaeder wrote:

[PATCH] D134744: [clang][Interp] Implement rem opcode

erichkeane accepted this revision. erichkeane added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/AST/Interp/Integral.h:213 + static bool rem(Integral A, Integral B, unsigned OpBits, Integral *R) { +*R = Integral(A.V % B.V); --

[PATCH] D134744: [clang][Interp] Implement rem opcode

tbaeder added inline comments. Comment at: clang/lib/AST/Interp/Integral.h:213 + static bool rem(Integral A, Integral B, unsigned OpBits, Integral *R) { +*R = Integral(A.V % B.V); erichkeane wrote: > Its a touch weird we return 'bool' in all these places,

[PATCH] D134744: [clang][Interp] Implement rem opcode

tbaeder updated this revision to Diff 463251. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134744/new/ https://reviews.llvm.org/D134744 Files: clang/lib/AST/Interp/ByteCodeExprGen.cpp clang/lib/AST/Interp/Integral.h clang/lib/AST/Interp/Interp.h clang/lib/AST/Interp/Opcodes.td

[PATCH] D134744: [clang][Interp] Implement rem opcode

erichkeane added inline comments. Comment at: clang/lib/AST/Interp/Integral.h:213 + static bool rem(Integral A, Integral B, unsigned OpBits, Integral *R) { +*R = Integral(A.V % B.V); Its a touch weird we return 'bool' in all these places, rather than an `

[PATCH] D134744: [clang][Interp] Implement rem opcode

tbaeder updated this revision to Diff 463245. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134744/new/ https://reviews.llvm.org/D134744 Files: clang/lib/AST/Interp/ByteCodeExprGen.cpp clang/lib/AST/Interp/Integral.h clang/lib/AST/Interp/Interp.h clang/lib/AST/Interp/Opcodes.td

[PATCH] D134744: [clang][Interp] Implement rem opcode

tbaeder created this revision. tbaeder added reviewers: aaron.ballman, erichkeane, shafik, tahonermann. Herald added a project: All. tbaeder requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Something easier to review I hope :) Implement an