I'm working on extending the OptimizeGlobalAddressOfMalloc to handle some more general cases. One of the cases is the null check of global pointer variables. It is disabled with https://reviews.llvm.org/rGb7cd291c1542aee12c9e9fde6c411314a163a8ea. This PR is to reenable it while fixing the original problem reported. The fix is to set the store value correctly when creating store for the new created global init bool symbol.
Only loosely relevant to this patch, but are we really expecting anyone to call the result of malloc? Or am I misreading this?
Maybe explicitly note here that the rewriting code has special handling for this case.
Not sure this interacts correctly with AllUsesOfValueWillTrapIfNull recursion. It looks like the rewriting code can't handle an icmp of a bitcast/GEP/PHI of a load.
I think GV->hasInitializer() must be true here?
The equality comparison here makes me a little nervous, in case the way we analyze bitcasts changes in the future. SI->getValueOperand()->isNullValue() would probably be more clear anyway.
The code that was meant to support the icmp comparison case.
Sinking the load seems wrong; how do we know we aren't sinking it past a store?
This to address the review comments: (1) Refine the code to handle ICmpInst when checking if AllUsesOfValueWillTrapIfNull, (2) Consider the GlobalVariable is initialized when creating StoreInst for the global init bool symbol, (3) Remove the unsafe code sinking when transforming the ICmpInst.
I think we can simply return false when passing to call argument, but I'll leave the code as it is for now. I'm going to extend the code for function call argument passing and function returns later in another patch,
Add restriction for operand 0 as the transformation only handles this kind of code pattern.
That's right, GV->hasInitializer() is true here. Modify the code as you suggested.
I agree, the sinking is not safe. Remove the comment, and the code sinking.
I'd like to see a little more test coverage, given all the different cases where the behavior of the transform changed.
Would !isa<Constant>(SI->getValueOperand())) work?
Thanks for the suggestion. I'll come back to add some test cases when I get time later.
Add two test cases - one is to test the icmp condition with non-null value, the other is to test the disablement of the sinking of load to the icmp location.
This won't work, as the "GV = malloc()" instruction has been transformed to "GV = &NewGV", and "&NewGV" is a Constant.
I'm not sure what comments I can add, as I actually don't know the logic here for the folding of the signed compares. The code has been like this from 2009 (the earliest I can found). Do you have any suggestions on how to handle all these signed compares?
We could probably just reject icmps with a signed predicate; frontends won't normally generate signed pointer comparisons anyway.
Alternatively, you could use some tortured logic here: the program doesn't actually capture the pointer, and you can't do anything useful just knowing the sign of the pointer. So just pretend the sign bit isn't set.
Thank you very much for the suggestions. I'll take the safe approach - rejecting the signed comparisons. Hopefully it won't have much impact on real programs.