Previously, genArrayAssignment is used for lowering assignment to
(sub)string in forall, which is abviously wrong. This supports lowering
assignment to (sub)string in forall when this assignment does not have
memory conflict. That is, in assignment statement (R1032: var = expr),
the scenario of the symbol in variable referenced in experssion is not
supported yet.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
- For the first test case (test_forall_with_string_assignment) in this pach, gfortran and ifort have the following results:
character(6) :: y, x = '123456'
forall (i=1:4)
y = x(i+1:i+1)
end forall
print *, y
end$ gfortran test.f90 && ./a.out test.f90:4:4: 4 | y = x(i+1:i+1) | 1 Warning: The FORALL with index ‘i’ is not used on the left side of the assignment at (1) and so might cause multiple assignment to this object 5 $ ifort test.f90 && ./a.out test.f90(4): error #6697: The leftside of a forall-assignment is invalid. [Y] y = x(i+1:i+1) ----^ compilation aborted for test.f90 (code 1)
With this patch, flang-new has the same results as gfortran, and it should be semantically correct.
- For the follwing test cases (which are TODO in this patch):
character(6) :: x, y = '123456'
equivalence(x,y)
forall (i=1:4)
x(i+2:i+2) = y(i+1:i+1)
print *, x
end character(6) :: x(2) = (/'123456', '123456'/)
forall (i=1:4)
x(1)(i+2:i+2) = x(1)(i+1:i+1)
end forall
print *, x
endThe solution in my mind is as follows:
%temp = fir.allocmem ...
fir.do_loop {
assign genExprAddr(assign.rhs, stmtCtx) to temp
}
fir.do_loop {
assign temp to genExprAddr(assign.lhs, stmtCtx)
}| flang/lib/Lower/Bridge.cpp | ||
|---|---|---|
| 1854 | So far this kind of analysis was done at the FIR level (in the array copy pass). I am not sure we should start to do it in both places just for substrings, we will have corner case bugs in the substring case because it will probably not be used very often (for instance, it seems your new function assignmentHasMemConflict does not consider the presence of POINTERs in the assignments, and variables associated via the Associate construct might also nee special consideration). @schweitz, what do you think ? | |
| flang/lib/Lower/Bridge.cpp | ||
|---|---|---|
| 1854 |
It's true. I am doing it here since I did not find FIR ops having the similar semantics as fir.array_modify and fir.array_merge_store for string. This patch is only the initial version and may not be the right solution.
Right. I missed them. I am thinking if we can support this corner case by take the string as one-dimensional one-sized array like the following: character(6) :: x(1) = (/'123456'/)
forall (i=1:4)
x(:)(i+2:i+2) = x(:)(i+1:i+1)
end forall
print *, x
endMaybe it needs more changes to the array Ops for supporting substring. | |
| flang/lib/Lower/Bridge.cpp | ||
|---|---|---|
| 1854 | Substrings are not really "more special" and should not be factored out of the existing framework. Substrings are in the class of array updates which require elemental merges of values between the old and new (in other words, they are partial updates on an array element), which has a different semantics than updates that replace whole values. The existing framework is intended to work in both cases regardless. The support of CHARACTER operations in array/where/forall contexts is lagging behind support for other types and the implementation needs to be completed. Sorry for any inconvenience. | |
Comment Actions
I abandon this PR for now since this is not the correct fix.
| flang/lib/Lower/Bridge.cpp | ||
|---|---|---|
| 1854 | Got it. Thanks for the explanations. This makes sense. It seems that the substrings are seldom used in real workloads. | |
So far this kind of analysis was done at the FIR level (in the array copy pass). I am not sure we should start to do it in both places just for substrings, we will have corner case bugs in the substring case because it will probably not be used very often (for instance, it seems your new function assignmentHasMemConflict does not consider the presence of POINTERs in the assignments, and variables associated via the Associate construct might also nee special consideration).
@schweitz, what do you think ?