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

Convert Olden/voronoi benchmark #51

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lenary
Copy link
Collaborator

@lenary lenary commented Jun 3, 2017

This is not yet complete, but I thought I'd push the branch anyway.

In particular, the bounds inference code can't cope with these macros. I'm not entirely sure why.

#define sym(a) ((QUAD_EDGE) (((uptrint) (a)) ^ 2*SIZE))
#define rot(a) ((QUAD_EDGE) ( (((uptrint) (a) + 1*SIZE) & ANDF) | ((uptrint) (a) & ~ANDF) ))
#define rotinv(a) ((QUAD_EDGE) ( (((uptrint) (a) + 3*SIZE) & ANDF) | ((uptrint) (a) & ~ANDF) ))
#define base(a) ((QUAD_EDGE) ((uptrint a) & ~ANDF))

@lenary
Copy link
Collaborator Author

lenary commented Jun 3, 2017

In particular, the rot and rotinv definitions seem to be the issue, as they use the macro parameter twice in the macro definition. Relevant Inference lines are here: https://github.com/Microsoft/checkedc-clang/blob/master/lib/Sema/SemaBounds.cpp#L700-L706

@dtarditi
Copy link
Contributor

dtarditi commented Jun 5, 2017

In the case of operations involving integers, the current spec says only one of the arguments can have bounds. I didn't know to choose which "bounds" wins when both of the arguments have bounds. However, we could require that if both arguments have bounds, they must be identical. This would handle the special case where the bounds for both arguments are derived from the same variable.

I had expected that a programmer would in general be able to resolve the situation of ambiguous bounds inference by assigning the argument that should have bounds to a variable with no bounds declared. However, that isn't possible to do in a macro the produces an expression. There's no way to introduce temporary variables and we'd have to worry about variable capture.

@lenary
Copy link
Collaborator Author

lenary commented Jun 6, 2017

So right now the two options if there are bounds on both sides are:

  • reject (which breaks in this code, and probably lots of masking code)
  • accept if the bounds expressions are equal

I propose a third option:

  • accept if the bounds are equal, and come from the same lvalue expression.

This should cover the above case, without accepting too much. Also means we don't have to inspect the bounds deeply for equality with respect to commutative/associative operators.

@lenary
Copy link
Collaborator Author

lenary commented Jun 6, 2017

So, I hacked a little on clang and the answer is that comparing for equality is going to be harder than I thought, especially as the bounds in this example aren't the same object, but do have equivalent expressions for upper and lower.

I hacked up a solution where I just chose the LHS if both are present (to see what would happen), and with a few changes this does build, but there's a runtime failure i have to investigate. Incoming commit with the changes to the benchmark, won't commit my change to clang, as it's not a good solution.

@dtarditi
Copy link
Contributor

dtarditi commented Jun 6, 2017

I don't think we need the third option. The point is to avoid ambiguity, and if the bounds expressions are equal, there is no ambiguity. Also, it would be hard to determine if the lvalue expression is the same. We could do it, but it requires a new analysis.

We will already have to deal with normalizing bounds expressions for redeclarations. We should use the same approach everywhere for checking equivalency of bounds expressions.

@lenary
Copy link
Collaborator Author

lenary commented Jun 6, 2017

It is now compiling if I change the compiler to always take the RHS (or the LHS) if both sides have bounds. I don't have a comparison for equality.

However, the output it gives is different to the reference output, which I need to investigate, which I'll do now.

@lenary
Copy link
Collaborator Author

lenary commented Jun 6, 2017

So of course the bounds were actually wrong, and have to also be expressed in terms of the bit-twiddling macros that give us so many issues.

I introduced a macro, #define quad_bounds(a) bounds(base(a), base(a) + 4) to define the bounds of a QUAD_EDGE structure (this is correct as QUAD_EDGE is always aligned to the closest 4-struct boundary). Unfortunately this gives us two issues.

  • The following declaration isn't allowed because var is not defined. This is almost certainly stopping cyclic dependencies in bounds declarations, but also stops us doing this bit-twiddling.
    QUAD_EDGE var : quad_bounds(var);
  • We can't yet define the correct bounds on the return types, as we haven't implemented our the return_value keyword. We'd like to be able to define functions something like so, eventually.
    QUAD_EDGE alloc_edge(void) : quad_bounds(return_value);

This means I'm going to shelve this conversion for the moment.

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.

4 participants