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

Adding bounds inference of the form count(i+1) #602

Merged
merged 3 commits into from
Jun 18, 2021
Merged

Conversation

Machiry
Copy link
Collaborator

@Machiry Machiry commented May 31, 2021

This will add a new heuristic, where the bounds of an array will be inferred as index + 1.

For instance, for the following code:

int getX(int *x) { return x[0]; }
int getY(int *x, int i) { return x[i]; }

The bounds will be inferred as follows:

int getX(_Array_ptr<int> x : count(0 + 1)) { return x[0]; }
int getY(_Array_ptr<int> x : count(i + 1), int i) { return x[i]; }

More details: #599

This improves the bounds inference of other benchmarks as well.

For yacr2 of ptrdist:

ulong ExistPathAboveVCG(_Array_ptr<nodeVCGType> VCG : count(below + 1), ulong above, ulong below)
{
    DFSClearVCG(VCG);
    DFSAboveVCG(VCG, above);
    return(VCG[below].netsAboveReached);
}

That further added bounds to other functions:

ulong VCV(_Array_ptr<nodeVCGType> VCG : count(check + 1), ulong check, ulong track, _Array_ptr<ulong> assign)
{
    ulong	net;
    ulong	vcv;
    vcv = 0;
    for (net = 1; net <= channelNets; net++) {
	if (assign[net]) {
	    if (assign[net] < track) {
		/*
		 * Above VCV?
		 */
		if (ExistPathAboveVCG(VCG, net, check)) {
		    vcv++;
		}
	    }
	    else if (assign[net] > track) {
		/*
		 * Below VCV?
		 */
		if (ExistPathAboveVCG(VCG, check, net)) {
		    vcv++;
		}
	    }
	    else {
		/*
		 * Above or Below VCV?
		 */
		if (ExistPathAboveVCG(VCG, net, check) || ExistPathAboveVCG(VCG, check, net)) {
		    vcv++;
		}
	    }
	}
    }
    return(vcv);
}

@mattmccutchen-cci
Copy link
Member

I would still like to have a realistic example program on record before adding this much code to 3C, but others can overrule me.

assert(!Changed && "New arrays needed bounds after inference");
TmpArrNeededBounds = ArrNeededBounds;
OuterChanged = !ArrNeededBounds.empty();
while (OuterChanged) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's changed here that requires and extra outer loop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed the organization of the code a little.
OuterLoop (Loop until nothing changes):

  • Loop until nothing changes: Propagate Bounds.
  • Loop untill nothing changes: Get bounds from likely bounds (e.g., i < n, arr[i], etc).

clang/lib/3C/ABounds.cpp Outdated Show resolved Hide resolved
clang/lib/3C/AVarBoundsInfo.cpp Show resolved Hide resolved
clang/test/3C/allarrays.c Show resolved Hide resolved
clang/test/3C/placements.c Show resolved Hide resolved
@@ -15,6 +15,6 @@ void foo(int *w) {
y[1] = 3;
int *z = realloc(y, 5 * sizeof(int));
//CHECK_NOALL: int *z = realloc<int>(y, 5 * sizeof(int));
//CHECK_ALL: _Array_ptr<int> z = realloc<int>(y, 5 * sizeof(int));
//CHECK_ALL: _Array_ptr<int> z : count(3 + 1) = realloc<int>(y, 5 * sizeof(int));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why doesn't realloc generate a bound on z?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As of now, we do not insert bounds because of realloc (we only handle malloc). Lets make another issue for this.

@@ -7,19 +7,19 @@

int **func(int **p, int *x) {
//CHECK_NOALL: int **func(int **p : itype(_Ptr<_Ptr<int>>), _Ptr<int> x) : itype(_Ptr<int *>) {
//CHECK_ALL: _Array_ptr<_Ptr<int>> func(_Array_ptr<_Ptr<int>> p, _Ptr<int> x) _Checked {
//CHECK_ALL: _Array_ptr<_Ptr<int>> func(_Array_ptr<_Ptr<int>> p : count(1 + 1), _Ptr<int> x) : count(1 + 1) _Checked {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The return bound here is incorrect, but maybe this is acceptable since we know bounds inference gets bounds wrong some times.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup. Let's make another issue for this. There are some issues with generating bounds for &(arr[1]) kinda expressions.

y[10] = 1;
}

int *foo(int *q) {
//CHECK: _Array_ptr<int> foo(_Array_ptr<int> q) {
//CHECK: _Array_ptr<int> foo(_Array_ptr<int> q) : count(10 + 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could q get the same bound as the return?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The return bounds do not flow to inside variables.

Copy link
Collaborator

@john-h-kastner john-h-kastner left a comment

Choose a reason for hiding this comment

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

OK. looks good.

Copy link
Member

@mattmccutchen-cci mattmccutchen-cci left a comment

Choose a reason for hiding this comment

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

Just recording my skepticism that this change should be made at all (per the pending discussion in #599) in a more obvious way, though of course I may be overruled, in which case we can dismiss my review here.

@Machiry
Copy link
Collaborator Author

Machiry commented Jun 16, 2021

@john-h-kastner I tested on yacr2 of ptrdist. We found many new bounds.
Example:
in vcg.c

ulong ExistPathAboveVCG(_Array_ptr<nodeVCGType> VCG : count(below + 1), ulong above, ulong below)
{
    DFSClearVCG(VCG);
    DFSAboveVCG(VCG, above);
    return(VCG[below].netsAboveReached);
}
ulong VCV(_Array_ptr<nodeVCGType> VCG : count(check + 1), ulong check, ulong track, _Array_ptr<ulong> assign)
{
...
if (ExistPathAboveVCG(VCG, net, check)) {

and there are other examples too.
I think this change is worth it.

(Recording justification)

@mattmccutchen-cci
Copy link
Member

With regrets for belaboring the point, this is the same example that you already posted on #599 and that I argued there wasn't convincing to me.

@Machiry
Copy link
Collaborator Author

Machiry commented Jun 18, 2021

Merging this for now. Zane will do a clean of the code and make it more streamlined.

@Machiry Machiry merged commit 2a96dfb into main Jun 18, 2021
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.

new bounds heuristic: one larger than the largest index accessed in the function?
3 participants