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

Fix isUnusedResult to work with clad::array #543

Closed
wants to merge 2 commits into from

Conversation

Daksh1603
Copy link

@Daksh1603 Daksh1603 commented Mar 15, 2023

Fixes #350

This pull request addresses an issue with the isUnusedResult function in CLAD, which currently does not correctly identify unused results for clad::array expressions.

To fix this issue , I have made some minor changes to the isUnusedResult function from the clad/Differentiator/VisitorBase.cpp file which explicity checks whether the passed expression is an instance of 'clad::array'.

@Daksh1603 Daksh1603 changed the title Fix extra lines generated when using clad::array. #350 Fix isUnusedResult to work with clad::array Mar 15, 2023
@ShounakDas101
Copy link
Contributor

ShounakDas101 commented Mar 16, 2023

what is " Expr " type?

@Daksh1603
Copy link
Author

Daksh1603 commented Mar 16, 2023

Hi @ShounakDas101 ,

Expr is basically a class from the Clang AST . It is capable of representing multiple C/C++ expressions like operators,literal . I myself am trying to figure out Clad and Clang and as you dwell into the code and documentation , more things will make sense. I might not be entirely right but this is what I think of Expr as of now . Please let me know if you find a better explanation for the following .

@ShounakDas101
Copy link
Contributor

ShounakDas101 commented Mar 16, 2023

Hi @Daksh1603
How did you check your code?
I tried running this code and a few variations

#include "clad/Differentiator/Differentiator.h"
#include

int main()
{
//int a[1];
//a[0]=0; //maybe change a[0] =3
clad::array a(3);

if(a[0])
{
	std::cout<<"true";
}
else
{
	std::cout<<"false";
}
return 1;

}

I dont think I can replicate the error properly with this nor did your changes seem to affect the outputs.

Also in the issue this is the example given

double arr(3); <-------- is this just a normal array declaration or smth else?
arr[1]; // <- isUnusedResult returns true for this statement

Note: for some reason iostream not showing up

@codecov
Copy link

codecov bot commented Mar 16, 2023

Codecov Report

Merging #543 (aaa9bdb) into master (7c80111) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #543   +/-   ##
=======================================
  Coverage   92.50%   92.50%           
=======================================
  Files          39       39           
  Lines        5600     5602    +2     
=======================================
+ Hits         5180     5182    +2     
  Misses        420      420           
Impacted Files Coverage Δ
lib/Differentiator/VisitorBase.cpp 97.64% <100.00%> (+0.01%) ⬆️
Impacted Files Coverage Δ
lib/Differentiator/VisitorBase.cpp 97.64% <100.00%> (+0.01%) ⬆️

@parth-07
Copy link
Collaborator

@Daksh1603 I was expecting some test changes as well that I don't see.

Can you also please answer to @ShounakDas101 questions?

@Daksh1603
Copy link
Author

Daksh1603 commented Mar 17, 2023

Hi @ShounakDas101 ,

Apologies for the late reply , as I had my mid semester test today .
I don't think the code snippet you mentioned is the right way to replicate this error . In my opinion , for you to replicate this error in it's entirety , try running the following code :

#include <iostream>
#include "clad/Differentiator/Differentiator.h"
#include "clad/Differentiator/Array.h"

double fn(double* a) {
  	clad::array<int> b(3);
	b[1];
	double arr1[3];
	arr1[1];
        return a[1];
}

int main() {
  clad::array<double> a(10); 
  auto darray_dx = clad::differentiate(fn,"a[0]");
  std::cout<<darray_dx.execute(a)<<"\n"; 
  return 0;
}

However , this is what I think is right and may not be absolutely correct .

@Daksh1603
Copy link
Author

Hi @parth-07 @vgvassilev ,
As I dwell into this problem more , I am discovering more vulnerabilities in my commit . The earlier solution which I implemented doesn't seem to work for me , but I couldn't find the flaw . I have 2 possible questions and wanted to ask you for help ->

  • I have two separate build directories for the CLAD plugin . One is the standard CLAD build and the other is my own forked CLAD build , to where I make changes . Now in order for me to inspect the changes I have made and whether the issue is really solved , I need to understand how can I swap between these two builds at ease . Usually , I just end up running the make && make install command for respective build directory . But perhaps , this might be the wrong way to go about it .
  • Secondly , I tried implementing an alternate solution for this , please have a look ->
if (auto array = dyn_cast<ArraySubscriptExpr>(E)) {
        if (auto variable = dyn_cast<VarDecl>(array->getBase()->getReferencedDeclOfCallee())) {
                if (variable->getNameAsString()== "clad::array") {
                    return !array->getBase()->isLValue();
                }
            }
        }

This seems like a reasonable approach but what concerns me is the fact that getReferencedDeclOfCallee() might end up pointing to the operator which is being overloaded instead . A little more guidance will be appreciated on this and I am confident that Ill end up fixing it at the earliest .

@parth-07
Copy link
Collaborator

  • I have two separate build directories for the CLAD plugin . One is the standard CLAD build and the other is my own forked CLAD build , to where I make changes . Now in order for me to inspect the changes I have made and whether the issue is really solved , I need to understand how can I swap between these two builds at ease . Usually , I just end up running the make && make install command for respective build directory . But perhaps , this might be the wrong way to go about it .

Why do you need two builds?
Ideally, you do not need two builds at all. You can have one build with multiple remotes.
And did you globally install Clad in your system? I would recommend locally installing Clad. That generally makes
things more clear. And this way, if you have multiple builds, then you can choose which Clad to use when running a program with Clad plugin, by passing the correct path to the Clad plugin library when running the clang command.

  • Secondly , I tried implementing an alternate solution for this , please have a look ->
if (auto array = dyn_cast<ArraySubscriptExpr>(E)) {
        if (auto variable = dyn_cast<VarDecl>(array->getBase()->getReferencedDeclOfCallee())) {
                if (variable->getNameAsString()== "clad::array") {
                    return !array->getBase()->isLValue();
                }
            }
        }

This seems like a reasonable approach but what concerns me is the fact that getReferencedDeclOfCallee() might end up pointing to the operator which is being overloaded instead . A little more guidance will be appreciated on this and I am confident that Ill end up fixing it at the earliest .

This solution has some major flaws. It seems it would only work for clad::array. Do pure array types have the same issue as of clad::array? If yes, then we should fix that as well. Here the important thing to check is, if an expression has some side effects, it does then we want to keep it. If not, then we can skip outputting them. I suspect there may be some Clang API to check if an expression has side-effects, but I am not sure at the moment.

@vgvassilev
Copy link
Owner

Let's close this PR as the issue was fixed in #797.

@vgvassilev vgvassilev closed this Mar 5, 2024
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.

Fix extra lines generated when using clad::array.
4 participants