This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Implement missing compound assign operators
ClosedPublic

Authored by tbaeder on Oct 31 2022, 4:46 AM.

Diff Detail

Event Timeline

tbaeder created this revision.Oct 31 2022, 4:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 4:46 AM
tbaeder requested review of this revision.Oct 31 2022, 4:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 4:46 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tbaeder updated this revision to Diff 471965.Oct 31 2022, 4:51 AM
aaron.ballman added inline comments.Nov 1 2022, 4:49 AM
clang/test/AST/Interp/literals.cpp
553
static_assert(IntRem(9, 0) == 12, ""); // Not constexpr
static_assert(IntRem(__INT_MIN__, -1) == 12, ""); // Not constexpr
572–573

Same suggestion for div as for rem

595

An overflow test would be good here.

tbaeder updated this revision to Diff 472503.Nov 1 2022, 10:12 PM
tbaeder marked 3 inline comments as done.

Precommit CI looks to have potentially found something interesting here.

clang/test/AST/Interp/literals.cpp
553

Missed the test for INT_MIN and -1?

tbaeder updated this revision to Diff 474181.Nov 9 2022, 12:49 AM
tbaeder marked an inline comment as done.

Precommit CI looks to have potentially found something interesting here.

There might be one of my local patches missing, but it looks like it's somehow not applying this patch, which is weird.

clang/test/AST/Interp/literals.cpp
553

for rem, clang doesn't diagnose anything: https://godbolt.org/z/rhe5ezc54

aaron.ballman added inline comments.Nov 9 2022, 7:04 AM
clang/test/AST/Interp/literals.cpp
553

That's a bug per https://eel.is/c++draft/expr.mul#4.sentence-3 because the resulting algebraic value is not representable in the type of the result.

591

This one should fail for the same reason as %

tbaeder updated this revision to Diff 474273.Nov 9 2022, 7:55 AM
tbaeder marked an inline comment as done.
tbaeder added inline comments.
clang/test/AST/Interp/literals.cpp
591

Works when replacing all the unsigneds with ints.

This revision is now accepted and ready to land.Nov 9 2022, 11:54 AM
tbaeder marked an inline comment as done.Nov 11 2022, 6:14 AM

Can't push this without https://reviews.llvm.org/D135750, since the variables in the test functions are used without being initialized.

shafik accepted this revision.Dec 20 2022, 10:10 PM

LGTM

clang/test/AST/Interp/literals.cpp
553

Ok, this bugged me enough I figured out what was going on and put up a PR: https://reviews.llvm.org/D140455

Really didn't expect this: https://lab.llvm.org/buildbot/#/builders/214/builds/5415

Command Output (stderr):
--
+ : 'RUN: at line 1'
+ /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/bin/clang -cc1 -internal-isystem /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/lib/clang/16/include -nostdsysteminc -fexperimental-new-constant-interpreter -std=c++11 -verify /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/clang/test/AST/Interp/literals.cpp
+ : 'RUN: at line 2'
+ /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/bin/clang -cc1 -internal-isystem /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/lib/clang/16/include -nostdsysteminc -fexperimental-new-constant-interpreter -std=c++20 -verify /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/clang/test/AST/Interp/literals.cpp
error: 'error' diagnostics seen but not expected: 
  File /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/clang/test/AST/Interp/literals.cpp Line 481: static assertion failed due to requirement 'BoolOr(false, true)': 
  File /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/clang/test/AST/Interp/literals.cpp Line 500: static assertion failed due to requirement 'BoolAnd(true, true)': 
  File /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/clang/test/AST/Interp/literals.cpp Line 521: static assertion failed due to requirement '!BoolXor(true, true)': 
  File /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/clang/test/AST/Interp/literals.cpp Line 523: static assertion failed due to requirement 'BoolXor(false, true)': 
4 errors generated.

I've seen s390x and AIX builders break here, probably a problem with big-endian machines?