Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instead of offsetting indices, set dimensions correctly in variable type #683

Closed
czgdp1807 opened this issue Jun 23, 2022 · 4 comments · Fixed by #685
Closed

Instead of offsetting indices, set dimensions correctly in variable type #683

czgdp1807 opened this issue Jun 23, 2022 · 4 comments · Fixed by #685
Labels
asr_pass ASR pass related changes asr ASR related changes

Comments

@czgdp1807
Copy link
Collaborator

ASR::expr_t *index_add_one(const Location &loc, ASR::expr_t *idx) {
// Add 1 to the index `idx`, assumes `idx` is of type Integer 4
ASR::expr_t *comptime_value = nullptr;
ASR::ttype_t *a_type = ASRUtils::TYPE(ASR::make_Integer_t(al, loc,
4, nullptr, 0));
ASR::expr_t *constant_one = ASR::down_cast<ASR::expr_t>(ASR::make_IntegerConstant_t(
al, loc, 1, a_type));
return ASRUtils::EXPR(ASR::make_IntegerBinOp_t(al, loc, idx,
ASR::binopType::Add, constant_one, a_type, comptime_value));
}

if (ASR::is_a<ASR::IntegerConstant_t>(*value) || ASR::is_a<ASR::Var_t>(*value)) {
ASR::ttype_t *itype = ASRUtils::TYPE(ASR::make_Integer_t(al, loc,
4, nullptr, 0));
dim.m_start = ASR::down_cast<ASR::expr_t>(ASR::make_IntegerConstant_t(al, loc, 1, itype));
dim.m_end = value;

The above code is hackish and it becomes to keep track whether to offset the index by 1 in an optimisation pass or not. The reason of confusion is,

  1. We are setting lower bound as 1 and upper bound as size in both Python and Fortran.
  2. We offsetting indices by 1 only in Python.
  3. We only have ASR while performing an optimisation pass. In Python we set lower and upper bounds same as Fortran but we offset index by 1 only in Python. Hence no clear differences between the two.

I suggest doing the following,

  1. Set lower bound as 0 and upper bound as size - 1 in Python. In Fortran its provided by the user and default setting in LFortran is correct.
  2. Don't offset indices by 1 in any LCompiler.
  3. In an optimisation pass set the bounds of the loop (which we are creating manually in an optimisation pass) always to 0 + lower_bound(dimension) and size - 1 + lower_bound(dimension). In Python lower_bound(dimension) is always 0, so no worries. In Fortran lower_bound(dimension) is by default 1 so loop bounds will transform to 1 and size automatically.

I discovered this problem while working on #675 and I can fix it in #675 itself.

@certik Please let me know what you think.

@czgdp1807 czgdp1807 added asr ASR related changes asr_pass ASR pass related changes labels Jun 23, 2022
@certik
Copy link
Contributor

certik commented Jun 23, 2022

See also: https://gitlab.com/lfortran/lfortran/-/issues/666

As discussed there, I think we should move to (size, start_index) representation of arrays. I don't have an opinion whether we should get rid of start_index, which would mean adjusting/offsetting all array indexing (there are pros/cons of doing that). So first let's keep the start_index. We should not offset any indices (that's the whole point of storing start_index in the first place).

@czgdp1807
Copy link
Collaborator Author

Well, my main concern is offsetting indices. Either you always do it in an optimisation pass or you never do it because once an ASR is formed you never know whether the source was Python or Fortran. So offsetting indices should never be done in any frontend irrespective of which array representation we are using.

Anyways would it be okay if I remove offsetting indices by 1 in LPython? Let me know.

@certik
Copy link
Contributor

certik commented Jun 24, 2022

Yes, we should not offset indices in array indexing. So yes, please remove any such code.

@czgdp1807
Copy link
Collaborator Author

Great. I slept on it. I think it won’t be good idea to do it in #675 as it will be out of context. I will do it in a subsequent PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
asr_pass ASR pass related changes asr ASR related changes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants