Skip to content

[NFC] Remove old BoxValue class and rename IrBoxValue to BoxValue #671

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

Merged
merged 1 commit into from
Mar 17, 2021

Conversation

jeanPerier
Copy link
Collaborator

Note there is a tiny bug fix in BoxValue::verify, I could not help noticing the && instead of || while reading the comments in the updated. Apart from that, sed did most of this PR.

After this PR, remaining clean-up around extended values that I see:

  • We need to clarify the status of Unboxed/ArrayBoxValue. Should we officially use them for non polymorphic derive types with no length parameters (F95 derived types) ? If yes, some comments/names need to be updated (e.g SymbolBox::Intrinsic). If not, may want new extended value category for them.
  • MutableBoxValue need to be added to ExtendedValue variant.

@jeanPerier jeanPerier force-pushed the jp-boxvalue-clean-up branch from 50bbefc to c9d5579 Compare March 16, 2021 12:57
Copy link

@schweitzpgi schweitzpgi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. (I made a comment, but it is unrelated to the scope of this PR, so no worries.)

@@ -404,6 +358,9 @@ class MutableBoxValue : public AbstractIrBox {
MutableProperties mutableProperties;
};

/// Used for triple notation (array slices)
using RangeBoxValue = std::tuple<mlir::Value, mlir::Value, mlir::Value>;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still used? (I thought we had removed it a while back.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is still used around subscript-triplet lowering here:

fir::RangeBoxValue genTriple(const Fortran::evaluate::Triplet &trip) {

It is a very localized use, so the definition could also be moved to ConvertExpr.cpp to avoid being exported in a public header.

Also make AbstractIrBox inherit from AbstractArrayBox to get the array
aspects from there.
@jeanPerier jeanPerier force-pushed the jp-boxvalue-clean-up branch from c9d5579 to 67cfbeb Compare March 17, 2021 07:09
@jeanPerier jeanPerier merged commit 7aeb24a into fir-dev Mar 17, 2021
@jeanPerier jeanPerier deleted the jp-boxvalue-clean-up branch March 17, 2021 07:10
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