Skip to content

[AST] Do not reorder non-constant operands#962

Open
bryanpkc wants to merge 1 commit into
flang-compiler:masterfrom
Huawei-CPLLab:fix-short-circuit-eval
Open

[AST] Do not reorder non-constant operands#962
bryanpkc wants to merge 1 commit into
flang-compiler:masterfrom
Huawei-CPLLab:fix-short-circuit-eval

Conversation

@bryanpkc

Copy link
Copy Markdown
Collaborator

Do not reorder non-constant operands, even for commutative opcodes. Doing so breaks short-circuit evaluation
of logical expressions, and can also cause side effects to occur out of order.

This PR includes a reproducer which, when compiled without the patch, would access an optional dummy argument before confirming its presence, leading to a segmentation fault.

...even for commutative opcodes. Doing so breaks short-circuit evaluation
of logical expressions, and can also cause side effects to occur out of
order.
@kiranchandramohan

Copy link
Copy Markdown
Collaborator

@bryanpkc Fortran does not guarantee short-circuit evaluation. This is a feature likely to come in the next revision of the standard using explicit keywords ANDTHEN, ORELSE. See the proposals below.
j3-fortran/fortran_proposals#19

@bryanpkc

Copy link
Copy Markdown
Collaborator Author

@kiranchandramohan Thanks for the link! I read "10.1.7 Evaluation of operands" of the Fortran 2018 standard and understood it as allowing short-circuit evaluation, but I was not aware that the language standard allowed operand evaluation in any order.

@bryanpkc

Copy link
Copy Markdown
Collaborator Author

The IF (PRESENT(x) .AND. some_expr(x)) pattern works fine when compiled with gfortran, and it is so common that I think Classic Flang ought to recognize the pattern and generate code that doesn't crash. If you agree, I'll work on that and submit a different patch.

@kiranchandramohan

Copy link
Copy Markdown
Collaborator

We can discuss this today.

@pawosm-arm

pawosm-arm commented Feb 10, 2021

Copy link
Copy Markdown
Collaborator

As a side note, I've checked that although cp2k and some other of workloads we're testing do use PRESENT (cp2k uses it alot), this is never used in a pattern where a value checked for its presence is referred from the same logical expression (like in IF (PRESENT(x) .AND. some_expr(x))). Apparently this is considered as a wrong doing and widely avoided.

@bryanpkc

Copy link
Copy Markdown
Collaborator Author

@pawosm-arm Yes, I have noticed that too. The failing example that we found came from a program written by a customer, who was perhaps used to gfortran's behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants