C/LLVM: Fixes with dict get

[Smit-create]

No description provided.

[czgdp1807]

We can't move ahead without enabling LLVM backend for any test. Please make it work with LLVM before C.

[certik]

That's a good point, the LLVM backend is a "lowering" backend, so it ensures our design is good. The C/C++/Julia backends are high level backends, so they don't reveal all issues. WASM is also a lowering backend.

[Smit-create]

Yep, that sounds right. Thanks, I fixed it.

[faze-geek]

Did some testing on this branch. The default value should only be considered when the key is not present in the dictionary.

def test_dict_11():
    d : dict[i32, i32]
    d = {1: 1, 2: 2, 3: 3}
    print(d.get(1, -1))
    print(d.get(3,100)) 

test_dict_11()

(lp) C:\Users\kunni\lpython>python try.py
1
3

(lp) C:\Users\kunni\lpython>src\bin\lpython try.py
-1
100

[faze-geek]

Also another strange case I came across -

def test_dict_11():
    d : dict[i32, str]
    d = {1:"1"}
    print(d.get(2,"0"))
    print(d.get(4,"0"))

test_dict_11()

(lp) C:\Users\kunni\lpython>src\bin\lpython try.py
0
KeyError: The dict does not contain the specified key

So should be made robust and tested that it does not give key error sometimes when key is not present in the dictionary. This might be a possible case of #1593 in dict

[czgdp1807]

I think result should only be allocated when def_value is not nullptr. I think you should not mix read_item with the logic of dict.get. Make a new method get_item and call it to implement dict.get. Direct access via [] should be done by read_item (which should be left untouched in this PR).

[Smit-create]

Presently, [] and dict.get are implemented with the same DictItem node in the ASR and we can re-use the same implementation just to extend it with default. I am not sure if it is a good idea to re-implement the same logic for get_item and read_item which effectively are doing the same job.

I think result should only be allocated when def_value is not nullptr.

It's also used on line 1597 which also makes it more generalized in the case when key_hash is matching but not the keys. This was the bug with the previous implementation where it raised an error for the case where key_hash matches but not the key while it didn't raise any error when the key is totally absent. This is also fixed using the current PR. So, I don't think separating two methods for implementing same logic is a good idea.

[czgdp1807]

Well in this PR itself we have seen (as pointed in #1700 (comment) and #1700 (comment)) how read_item is also affected when you implement dict.get inside it. So de-coupling if-else-if checks

I am not sure if it is a good idea to re-implement the same logic for get_item and read_item which effectively are doing the same job.

If code repetition is the issue then you can solve it by using a macro, or a utility method and call it from read_item and get_item.

Presently, [] and dict.get are implemented with the same DictItem node in the ASR and we can re-use the same implementation just to extend it with default

Won't be the case very soon. We will use IntrinsicFunction to implement, dict.item and dict.get separately.

It's also used on line 1597 which also makes it more generalized in the case when key_hash is matching but not the keys.

In https://github.com/lcompilers/lpython/blob/c935c96b50abeb0553213904a9f96247f6f78421/src/libasr/codegen/llvm_utils.cpp, AFAICT, its only used when def_value != nullptr,

lpython/src/libasr/codegen/llvm_utils.cpp

Lines 1596 to 1598 in c935c96

	if (def_value != nullptr) {
	LLVM::CreateStore(*builder, LLVM::CreateLoad(*builder, def_value), result);
	} else {

Again, my POV is still the same read_item and get_item should be different methods. However they can call common utility methods or macros to execute the common logic. if (some_arg_is_present) and then else always indicate that de-coupling is needed.

This was the bug with the previous implementation where it raised an error for the case where key_hash matches but not the key while it didn't raise any error when the key is totally absent.

Please add a test for it and fix the read_item and get_item methods accordingly.

[czgdp1807]

LGTM. Before merging, please ensure our dict benchmarks don't degrade due to this PR.

[Smit-create]

please ensure our dict benchmarks don't degrade due to this PR.

Slight changes are expected because of an extra if-else check in the last commit which was a bug.

[czgdp1807]

This should only happen in safe or debug modes of LPython. In fast mode one shouldn't catch these kind of errors. See, #1139.

I think following the same pattern as raising index errors in lists should work.

[Smit-create]

Thanks, updated this with even dedicated methods for default and bound checking which leaves the original method untouched (removed some if-else checks) and so its performance will be >= existing performance.