Hi,
in a recent review [0], Florian Hahn helped me to realize something that
was rather surprising to me:
The widely popular and very useful function
llvm::Value::stripPointerCasts() can return a value with a different
bit pattern than the input.
Now, I think this should not be the case but I want the hear other
opinions. Before I go on, please not that there is at least one location
in the code base [1] that makes a wrong assumption about the bit pattern
preservation.
The Problem
Hi,
in a recent review [0], Florian Hahn helped me to realize something that
was rather surprising to me:
The widely popular and very useful function
llvm::Value::stripPointerCasts() can return a value with a different
bit pattern than the input.
Now, I think this should not be the case but I want the hear other
opinions. Before I go on, please not that there is at least one location
in the code base [1] that makes a wrong assumption about the bit pattern
preservation.
If there's really only one place that gets this wrong (or only a few), I'm inclined to suggest option (1) below and fix this one place.
Matt, any thoughts on this?
-Hal
The Problem
Yes, I’ve wanted a version which strips addrspacecast and which doesn’t before, so +1
-Matt
Hi,
in a recent review [0], Florian Hahn helped me to realize something that
was rather surprising to me:
The widely popular and very useful function
llvm::Value::stripPointerCasts() can return a value with a different
bit pattern than the input.
Now, I think this should not be the case but I want the hear other
opinions. Before I go on, please not that there is at least one location
in the code base [1] that makes a wrong assumption about the bit pattern
preservation.
If there's really only one place that gets this wrong (or only a few), I'm inclined to suggest option (1) below and fix this one place.
Matt, any thoughts on this?
-Hal
Yes, I’ve wanted a version which strips addrspacecast and which doesn’t before, so +1
Do you have an opinion on which should be the default?
-Hal
-Matt
Not particularly. The current name sounds to me like it would strip any casts it possibly can. Maybe most uses should now be converted to a newly named stripTrivialPointerCasts?
-Matt
>
> Do you have an opinion on which should be the default?
>
> -Hal
>
>Not particularly. The current name sounds to me like it would strip
any casts it possibly can. Maybe most uses should now be converted to
a newly named stripTrivialPointerCasts?-Matt
I can certainly introduce a new stripTrivialPointerCasts() function that
will keep the bit pattern while stripping casts. Wrt to the options I
pointed out earlier (see below), this would then be 2). Afterwards, we
might want to go through all the stripPointerCasts() users to make sure
they do not actually want to call stripTrivialPointerCasts() instead.
1) Make stripPointerCasts not look through address space casts. A new
function (or parameter) would then be introduced to do the same thing
_and_ peel of address space casts.
2) Keep looking through address space casts in stripPointerCasts. A new
function (or parameter) would then be introduced to do the same but with
the guarantee that the bit pattern is preserved.
+1 to having both modes, well, if you find a case which is correct for the as-cast traversing one at least.
The default should probably be the non-as-cast traversing one for correctness.
Philip