Create "char buffer[...]" for "i8[...]" typed arrays #1346
Conversation
| array = CreateLoad(array); | ||
| } | ||
| bool is_data_only = is_argument && !ASRUtils::is_dimension_empty(m_dims, n_dims); | ||
| is_data_only = is_data_only || is_i8_array; |
There was a problem hiding this comment.
In here I am not sure we should special case i8. I think there is nothing special about i8, we should have exactly the same handling for all integer (and real and complex) arrays.
There was a problem hiding this comment.
Well, I guess the requirement was to create data only array for i8 if its a BindC. See #1333
| if( !force_declare ) { | ||
| force_declare_name = std::string(v.m_name); | ||
| } | ||
| sub = "char " + force_declare_name + dims; |
There was a problem hiding this comment.
Why not using int8_t ? It seems that's cleaner and consistent with i16, i32, i64, etc.
There was a problem hiding this comment.
I think in the issue it was mentioned to create char buffer[64] for buffer: i8[64].
|
I reviewed it commit by commit, it's clean, but I don't like the special casing for i8. If we can't determine this robustly ourselves, we might need to annotate the array in the source code, perhaps change I know we are struggling with exactly this issue in LFortran also at this very moment, when to use a descriptor and when not. This just adds to the complexity of the "ad-hoc" decision in the backend, and if the decision is this complex, then I think it should be part of ASR itself and the optimizer will choose, or even the frontend (initially). |
+1. I also didn't like it while implementing this. However from what I understood by reading #1333, and #1333 (comment), I thought it was needed only for My suggestion would be to always create data only arrays for fixed sizes and use descriptors otherwise. |
|
See #1350 to discuss this more. (There is nothing special about i8.) |
@certik Note that avoiding descriptors is impossible (as far as I can think) for "i8[:]" annotation, because we need to store lower and upper bounds. Hence I implemented this feature only for "i8[some_compile_time_dimension]".