[Flang][Runtime] Fix implicit conversion warning when targeting 32bit… #99465
Conversation
llvmbot
added
flang:runtime
flang
|
@llvm/pr-subscribers-flang-runtime Author: None (serge-sans-paille) Changes… architecture On 32 bit systems, TypeParameterValue is 64bit wide while CFI_index_t is 32bit wide. Full diff: https://github.com/llvm/llvm-project/pull/99465.diff 1 Files Affected:
diff --git a/flang/runtime/derived.cpp b/flang/runtime/derived.cpp
index 0d9e033df4e27..e14aa2e927bde 100644
--- a/flang/runtime/derived.cpp
+++ b/flang/runtime/derived.cpp
@@ -90,8 +90,25 @@ RT_API_ATTRS int Initialize(const Descriptor &instance,
comp.derivedType() && !comp.derivedType()->noInitializationNeeded()) {
// Default initialization of non-pointer non-allocatable/automatic
// data component. Handles parent component's elements. Recursive.
+<<<<<<< Updated upstream
SubscriptValue extents[maxRank];
GetComponentExtents(extents, comp, instance);
+=======
+ SubscriptValue extent[maxRank];
+ const typeInfo::Value *bounds{comp.bounds()};
+ for (int dim{0}; dim < comp.rank(); ++dim) {
+ auto lb = bounds[2 * dim].GetValue(&instance).value_or(0);
+ auto ub = bounds[2 * dim + 1].GetValue(&instance).value_or(0);
+ if (ub >= lb) {
+ auto bound_diff = ub - lb;
+ assert(bound_diff <= std::numeric_limits<SubscriptValue>::max() &&
+ "array bound overflow");
+ extent[dim] = bound_diff;
+ } else {
+ extent[dim] = 0;
+ }
+ }
+>>>>>>> Stashed changes
StaticDescriptor<maxRank, true, 0> staticDescriptor;
Descriptor &compDesc{staticDescriptor.descriptor()};
const typeInfo::DerivedType &compType{*comp.derivedType()};
|
serge-sans-paille
force-pushed
the
fix/flang-runtime-extent
branch
from
1f84a53 to
d004a84
Compare
flang/runtime/derived.cpp
Outdated
| auto ub = bounds[2 * dim + 1].GetValue(&derivedInstance).value_or(0); | ||
| if (ub >= lb) { | ||
| auto bound_diff = ub - lb; | ||
| assert(bound_diff <= std::numeric_limits<SubscriptValue>::max() && |
There was a problem hiding this comment.
Flang runtime code uses RUNTIME_CHECK macro to control how asserts are emitted inside the runtime.
I am not sure if the check should be in the runtime though. @klausler:
- Does the frontend allow constructing array component whose extents do not fit in SubscriptValue (I guess with PDTs that may be hard to enforce in semantics)?
- Is it intended for
SubscriptValueto be 32bit whileTypeParameterValueis 64bit on 32bit machine?
There was a problem hiding this comment.
There's no consideration for 32-bit support in the frontend as far as I know, and it would be quite a project to try to add it.
There was a problem hiding this comment.
I assume that for a 32 bit target, we should never meet an overflow in subscript computation. But we could imagine targeting a 64 bit target from a 32 bit target (although flang doesn't compile from 32 bit arch at the moment), I guess that's why TypeParameterValue is 64 bit wide (?)
There was a problem hiding this comment.
A few words on the motivation : there are a few projects around there that compiles Lapack to wasm through a modified flang, e.g. https://github.com/r-wasm/flang-wasm . I'm trying to upstream the parts that could be relevant / beneficial to flang as a whole, without being too intrusive.
There was a problem hiding this comment.
I've updated the commit description and moved to a static_cast to reflect my understanding.
serge-sans-paille
force-pushed
the
fix/flang-runtime-extent
branch
from
d004a84 to
63d8d94
Compare
There was a problem hiding this comment.
I assume that for a 32 bit target, we should never meet an overflow in subscript computation.
It seems a correct assumption to me, SubscriptValue should just be 64bit on this target if one intended to support large susbcripts on 32bit target. Although as @klausler mentioned 32bit targets have not been tested by us.
A few words on the motivation : there are a few projects around there that compiles Lapack to wasm through a modified flang, e.g. https://github.com/r-wasm/flang-wasm . I'm trying to upstream the parts that could be relevant / beneficial to flang as a whole, without being too intrusive.
Thanks for that.
flang/runtime/derived.cpp
Outdated
| SubscriptValue ub{ | ||
| bounds[2 * dim + 1].GetValue(&derivedInstance).value_or(0)}; | ||
| extents[dim] = ub >= lb ? ub - lb + 1 : 0; | ||
| auto lb = bounds[2 * dim].GetValue(&derivedInstance).value_or(0); |
There was a problem hiding this comment.
nit: use braces init
In the runtime the style follows flang front-end coding style: https://github.com/llvm/llvm-project/blob/main/flang/docs/C%2B%2Bstyle.md
The whole point of using braces init is to get warning when narrowing like it was the case so that we can asses and discuss these situation.
flang/runtime/derived.cpp
Outdated
| auto lb = bounds[2 * dim].GetValue(&derivedInstance).value_or(0); | ||
| auto ub = bounds[2 * dim + 1].GetValue(&derivedInstance).value_or(0); | ||
| if (ub >= lb) { | ||
| auto bound_diff = ub - lb; |
There was a problem hiding this comment.
I think the +1 was dropped by mistake. I advise going back to the ternary and put the static_cast in around ub - lb + 1. It is easier to read for me at least.
serge-sans-paille
force-pushed
the
fix/flang-runtime-extent
branch
from
63d8d94 to
4fd8226
Compare
… architecture On 32 bit systems, TypeParameterValue is 64bit wide while CFI_index_t is 32bit wide. The cast from the internal representation (64bit) to the runtime representation (32bit) is fine as there shouldn't be a subscript of more than 32bits on a 32bit architecture.
serge-sans-paille
force-pushed
the
fix/flang-runtime-extent
branch
from
4fd8226 to
dcf1634
Compare
|
Update done. There's indeed a mention of |
#99465) Summary: … architecture On 32 bit systems, TypeParameterValue is 64bit wide while CFI_index_t is 32bit wide. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251184
… architecture
On 32 bit systems, TypeParameterValue is 64bit wide while CFI_index_t is 32bit wide.