enh : Create physical string descriptor to be used with allocatable strings #4918

assem2002 · 2024-09-26T12:15:57Z

Towards #2257
In separate PR I'll utilize size and capacity in lfortran_alloc() , lfortran_strcpy(), lfortran_string_write()

gxyd · 2024-09-27T11:43:47Z

Can you please help me understand the below ASR changes? (I've put in comments in ASR itself for what my question is)

On your branch, for the below program:

program main
    implicit none
    character(len=:), allocatable :: str1
    character(len=:), allocatable :: str2(:)
end program main

below is the ASR generated using lfortran test.f90 --show-asr:

(TranslationUnit
    (SymbolTable
        1
        {
            main:
                (Program
                    (SymbolTable
                        2
                        {
                            str1:
                                (Variable
                                    2
                                    str1
                                    []
                                    Local
                                    ()
                                    ()
                                    Default
                                    (Allocatable
                                        (Character 1 -2 () DescriptorString)  ;; this seems alright to me
                                    )
                                    ()
                                    Source
                                    Public
                                    Required
                                    .false.
                                ),
                            str2:
                                (Variable
                                    2
                                    str2
                                    []
                                    Local
                                    ()
                                    ()
                                    Default
                                    (Allocatable
                                        (Array
                                            (Character 1 -2 () PointerString)   ;; is this expected to be DescriptorArray or PointerString?
                                            [(()
                                            ())]
                                            DescriptorArray
                                        )
                                    )
                                    ()
                                    Source
                                    Public
                                    Required
                                    .false.
                                )
                        })
                    main
                    []
                    [(ImplicitDeallocate
                        [(Var 2 str1)
                        (Var 2 str2)]
                    )]
                )
        })
    []
)

gxyd · 2024-09-27T11:45:37Z

src/libasr/asr_utils.h

@@ -196,6 +196,12 @@ static inline ASR::ttype_t* type_get_past_array_pointer_allocatable(
            type_get_past_pointer(f)));
 }

+static inline bool is_allocatable_string(ASR::ttype_t* t){
+    return ASR::is_a<ASR::Character_t>(*ASRUtils::type_get_past_array_pointer_allocatable(t)) &&


Can we also maybe put an assert that it's an Allocatable?

That's much safer. I'll do that.
Do you think We should add such an assertion in ASR verify? to make sure any character type with physicalType = DescriptorString is an allocatable

Do you think We should add such an assertion in ASR verify? to make sure any character type with physicalType = DescriptorString is an allocatable

Yes, that would be nice. If my understanding is right, that should be the case.

Yes definitely put this into verify(). All "assumptions" should be checked by verify().

src/libasr/codegen/asr_to_llvm.cpp

assem2002 · 2024-09-27T12:12:32Z

(Allocatable
                                        (Array
                                            (Character 1 -2 () PointerString)   ;; is this expected to be DescriptorArray or PointerString?
                                            [(()
                                            ())]
                                            DescriptorArray
                                        )
                                    )
                                    ()

It's a design detail. but I didn't see a benefit for size and capacity here as you would do something like that to allocate the array allocate(character(2) :: str2(2) ) which allocates a an array of 2 strings and each string is allocated to be of fixed length 2 and not to be extended unless you reallocate the array with new string length. If we use a DescriptorString instead of PointerString, I think it won't benefit us in terms of performance.

certik · 2024-09-27T14:50:27Z

@assem2002 let me know when this is ready for review from me.

assem2002 · 2024-09-27T15:14:26Z

@certik it's ready. the new physical type works, I'll need to utilize size and capacity in another PR.
maybe you can wait till I finish some of @gxyd suggestions and resolve the conflicts.

certik · 2024-09-27T15:35:34Z

Go ahead and resolve the conflicts.

certik · 2024-09-27T15:49:07Z

Ok, so there is one fundamental problem here that we need to fix. Consider, let's say, this test: https://github.com/assem2002/lfortran/edit/descriptor_string_FEEDBACK/tests/reference/asr-string_04-2ebc0e6.stdout?pr=%2Flfortran%2Flfortran%2Fpull%2F4918

In here, we have the str variable, declared like this:

                            str:
                                (Variable
                                    2
                                    str
                                    []
                                    Local
                                    ()
                                    ()
                                    Default
                                    (Allocatable
                                        (Character 1 -2 () DescriptorString)
                                    )
                                    ()
                                    Source
                                    Public
                                    Required
                                    .false.
                                ),

And then we assign to it like this:

                    (Assignment
                        (Var 2 str)
                        (StringConstant
                            "abcd"
                            (Character 1 4 () PointerString)
                        )
                        ()
                    )

The types here do not match. The LHS of the assignment is of type (Allocatable (Character 1 -2 () DescriptorString)), the RHS is of type (Character 1 4 () PointerString), these are fundamentally not compatible types. There is one issue with the Allocatable handling, let's ignore that for now, I think we allow the mismatch (LHS can be Allocatable, while RHS is not), that's a separate issue. But the types themselves are (Character 1 -2 () DescriptorString) and (Character 1 4 () PointerString) and these are not compatible. The -2 is compatible with 4, but DescriptorString is not compatible with PointerString. You can't just assign it. You have to insert a cast from a PointerString to DescriptorString.

So there are two issues to fix:

check in verify() that the types are compatible and that LHS and RHS always agrees with the physical type. Including passing strings to functions.
And then fix the PR to pass verify() by inserting casts.

assem2002 · 2024-09-27T17:21:33Z

@certik so the key goal here is to make clean ASR, right? as using assignment statement with incompatible sides aren't an issue in the backend we just use runtime function lfortran_strcpy(char** lhs, char* rhs, int64 size_lhs[if descriptorString], int64 capacity_lhs[if descriptorString]). and we usually fetch the char* out of the stringDescriptor to be used freely in the codebase and with the runtime function unless we encounter lfortran_alloc() , lfortran_strcpy(), lfortran_string_write() which require fetching size and capacity to be extended.

certik · 2024-09-27T18:18:10Z

Clean ASR and clean backend. When converting descriptor to pointer, you just grab the pointer, but you do it in the visit_cast in the backend (when casting from pointer to descriptor, you need to create a descriptor, in the visit_cast). So in assignment you only assign either two pointers or two descriptors, which is straightforward.

assem2002 · 2024-09-28T12:11:38Z

Yeah I think explicit ASR casting is better way to to assignments and argument passing. This may even can get rid of the visit_expr_wrapper changes that Gaurav and I were talking about above.
So..

I'll make casting node that's similar to how array cast node is structured.
I'll do casting when needed. AST To ASR should be sufficient to catch the needed places where I should cast, right?

certik · 2024-09-28T20:35:57Z

Yes, exactly.

AST To ASR should be sufficient to catch the needed places where I should cast, right?

Yes.

certik · 2024-10-03T16:02:51Z

src/libasr/asr_utils.h

+        ASRUtils::type_get_past_array_pointer_allocatable(t))->m_physical_type == ASR::string_physical_typeType::DescriptorString; 
+}
+
+static inline ASR::expr_t* cast_string_pointer_to_descriptor(Allocator& al, ASR::expr_t* string){


Add documentation:

This functions casts if necessary.

certik · 2024-10-03T16:10:46Z

Let's document in the String type in doc/... when each physical type is used. Currently:

if it is allocatalbe, it is DescriptorString
otherwise it is PointerString

assem2002 · 2024-10-08T06:28:06Z

The PR is ready for review. I'm pushing documentation, It'd be helpful to read it before reviewing this.

gxyd · 2024-10-08T15:47:53Z

I'm sorry I forgot about this today, I'll take a look tomorrow morning.

certik · 2024-10-10T15:57:54Z

doc/src/asr/asr_nodes/type_nodes/StringPhysicalType.md

+
+- **DescriptorString :** 
+	- It's an LLVM struct to hold information about data, size and capacity.
+	- It's represented in LLVM IR as : `{char*, int64, int64}`. The benefit from having `size` and `capacity` is that it gives us the ability to have dynamic string that's not computationally expensive, just like `std::vector` in c++. So the key points are:


Suggested change

- It's represented in LLVM IR as : `{char*, int64, int64}`. The benefit from having `size` and `capacity` is that it gives us the ability to have dynamic string that's not computationally expensive, just like `std::vector` in c++. So the key points are:

- It's represented in LLVM IR as : `{char*, int64, int64}`. The benefit from having `size` and `capacity` is that it gives us the ability to have dynamic string that's not computationally expensive, just like `std::string` (and `std::vector`) in c++. So the key points are:

certik · 2024-10-10T15:58:32Z

doc/src/asr/asr_nodes/type_nodes/StringPhysicalType.md

+	- It's represented in LLVM IR as : `{char*, int64, int64}`. The benefit from having `size` and `capacity` is that it gives us the ability to have dynamic string that's not computationally expensive, just like `std::vector` in c++. So the key points are:
+		- `size` avoids calling `strlen` when we want to know the size of a string. 
+		- `capacity` gives us the flexibility of extending the string without losing performance by doubling the memory location every time we run out of allocated memory space.
+	- DescriptorString must always be an allocatable.


Add here when in ASR the DescriptorString is (or should be) used, and when PointerString should be used. See #4918 (comment).

Currently:

if it is allocatable, it is DescriptorString

otherwise it is PointerString

Yes, a fortran example would be nice as well, just the way we've it for StringPhysicalCast above (that was quite easy to understand)

certik · 2024-10-10T16:03:56Z

This is ready for review. This PR does one change: for allocatable strings it uses DescriptorString physical type now.

@gxyd, @Pranavchiku can you please carefully review this PR? At high level it looks good to me, we just need to check all details.

…apacity with the runtime functions

…hysical descriptor string

…g Types + minor change in string_section_assign

…not value (function return by value not pointer to avoid dangling pointer)

…e we use it)

…_ function

… yourself)

assem2002 · 2024-10-29T04:52:17Z

ready to review.

certik · 2024-10-30T15:54:22Z

@gxyd, @Pranavchiku can you please review this?

certik

I think this PR is good. We can improve upon it later as needed.

gxyd · 2024-11-01T02:48:38Z

Sorry, I didn't get the time to review it.

certik · 2024-11-01T04:43:19Z

No worries, we reviewed it at the phone call yesterday.

assem2002 force-pushed the descriptor_string_FEEDBACK branch from 06b154f to fba28db Compare September 26, 2024 21:42

assem2002 marked this pull request as ready for review September 26, 2024 23:27

assem2002 changed the title ~~enh : Create physical string descriptor to be use with allocatable strings~~ enh : Create physical string descriptor to be used with allocatable strings Sep 26, 2024

gxyd reviewed Sep 27, 2024

View reviewed changes

assem2002 marked this pull request as draft September 28, 2024 20:54

assem2002 force-pushed the descriptor_string_FEEDBACK branch from efd1f1f to 929c571 Compare October 3, 2024 15:41

certik reviewed Oct 3, 2024

View reviewed changes

assem2002 force-pushed the descriptor_string_FEEDBACK branch 3 times, most recently from 3457583 to 4123d10 Compare October 8, 2024 05:16

assem2002 marked this pull request as ready for review October 8, 2024 06:24

assem2002 force-pushed the descriptor_string_FEEDBACK branch from d23cf94 to 63e376f Compare October 9, 2024 05:24

certik reviewed Oct 10, 2024

View reviewed changes

certik requested review from gxyd and Pranavchiku October 10, 2024 16:02

assem2002 added 20 commits October 28, 2024 20:56

Create the physical descriptorString + start utilizing its size and c…

eb63d2b

…apacity with the runtime functions

Minor : use ndim initialized to 0 + add conditions if the string is p…

784bf86

…hysical descriptor string

Deallocate string that's descriptorString

f62b589

Create stringPhysicalCast visitor to cast between the 2 physicalStrin…

41be24d

…g Types + minor change in string_section_assign

Always use stringDescriptor variable in the code base as pointer and …

be01e31

…not value (function return by value not pointer to avoid dangling pointer)

Add documentation for stringPhysicalType + stringPhysicalCast

93f1f38

minor change in documentation

386e487

minor change in stringPhysicalType.md

8099c61

make sure to fetch internal character_t type

2a3431f

check and Cast in associate node

e7c3937

minor comment removal

6a61294

Add associate test for testing descriptorString to be RHS

7c0281c

minor in stringPhysicalCast.md

688f01a

Modify the docs : some examples and minor typos

24fb6fc

Use cast functions direclty (it doesn't check - we should check befor…

1c766c3

…e we use it)

minor : add assertions when passing an expression node to cast_string…

e0a1371

…_ function

Changes in documentation

0b8bd1f

Remove if condition (if you want to use cast node, you've to check it…

777edb2

… yourself)

Update reference tests

7c0a6e5

don't cast string if it's an arg for 'present' intrinsic

5e64d08

assem2002 force-pushed the descriptor_string_FEEDBACK branch from 7b2a524 to 5e64d08 Compare October 28, 2024 17:56

assem2002 mentioned this pull request Oct 31, 2024

Fix array constructor function #5216

Merged

certik approved these changes Oct 31, 2024

View reviewed changes

Merge branch 'main' into descriptor_string_FEEDBACK

b20f84b

certik enabled auto-merge (squash) October 31, 2024 16:11

certik merged commit edb2255 into lfortran:main Oct 31, 2024
36 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enh : Create physical string descriptor to be used with allocatable strings #4918

enh : Create physical string descriptor to be used with allocatable strings #4918

assem2002 commented Sep 26, 2024 •

edited

Loading

gxyd commented Sep 27, 2024

gxyd Sep 27, 2024

assem2002 Sep 27, 2024 •

edited

Loading

gxyd Sep 27, 2024

certik Sep 27, 2024

assem2002 commented Sep 27, 2024 •

edited

Loading

certik commented Sep 27, 2024

assem2002 commented Sep 27, 2024 •

edited

Loading

certik commented Sep 27, 2024

certik commented Sep 27, 2024

assem2002 commented Sep 27, 2024 •

edited

Loading

certik commented Sep 27, 2024

assem2002 commented Sep 28, 2024 •

edited

Loading

certik commented Sep 28, 2024

certik Oct 3, 2024

certik commented Oct 3, 2024

assem2002 commented Oct 8, 2024 •

edited

Loading

gxyd commented Oct 8, 2024

certik Oct 10, 2024

certik Oct 10, 2024 •

edited

Loading

gxyd Oct 11, 2024

certik commented Oct 10, 2024

assem2002 commented Oct 29, 2024

certik commented Oct 30, 2024

certik left a comment

gxyd commented Nov 1, 2024

certik commented Nov 1, 2024

	- It's represented in LLVM IR as : `{char*, int64, int64}`. The benefit from having `size` and `capacity` is that it gives us the ability to have dynamic string that's not computationally expensive, just like `std::vector` in c++. So the key points are:
	- It's represented in LLVM IR as : `{char*, int64, int64}`. The benefit from having `size` and `capacity` is that it gives us the ability to have dynamic string that's not computationally expensive, just like `std::string` (and `std::vector`) in c++. So the key points are:

enh : Create physical string descriptor to be used with allocatable strings #4918

enh : Create physical string descriptor to be used with allocatable strings #4918

Conversation

assem2002 commented Sep 26, 2024 • edited Loading

gxyd commented Sep 27, 2024

gxyd Sep 27, 2024

Choose a reason for hiding this comment

assem2002 Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

gxyd Sep 27, 2024

Choose a reason for hiding this comment

certik Sep 27, 2024

Choose a reason for hiding this comment

assem2002 commented Sep 27, 2024 • edited Loading

certik commented Sep 27, 2024

assem2002 commented Sep 27, 2024 • edited Loading

certik commented Sep 27, 2024

certik commented Sep 27, 2024

assem2002 commented Sep 27, 2024 • edited Loading

certik commented Sep 27, 2024

assem2002 commented Sep 28, 2024 • edited Loading

certik commented Sep 28, 2024

certik Oct 3, 2024

Choose a reason for hiding this comment

certik commented Oct 3, 2024

assem2002 commented Oct 8, 2024 • edited Loading

gxyd commented Oct 8, 2024

certik Oct 10, 2024

Choose a reason for hiding this comment

certik Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

gxyd Oct 11, 2024

Choose a reason for hiding this comment

certik commented Oct 10, 2024

assem2002 commented Oct 29, 2024

certik commented Oct 30, 2024

certik left a comment

Choose a reason for hiding this comment

gxyd commented Nov 1, 2024

certik commented Nov 1, 2024

assem2002 commented Sep 26, 2024 •

edited

Loading

assem2002 Sep 27, 2024 •

edited

Loading

assem2002 commented Sep 27, 2024 •

edited

Loading

assem2002 commented Sep 27, 2024 •

edited

Loading

assem2002 commented Sep 27, 2024 •

edited

Loading

assem2002 commented Sep 28, 2024 •

edited

Loading

assem2002 commented Oct 8, 2024 •

edited

Loading

certik Oct 10, 2024 •

edited

Loading