This patch introduces a new abstract attribute AANoUndef which corresponds to noundef IR attribute and deduce them.
Details
Diff Detail
Event Timeline
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
---|---|---|
7238 | A value that is used as a divisor of div/rem can still have undef bits. a = undef | 1 ; a is not noundef because it has undef bits x = udiv 100, a ; this is not UB Can we use isGuaranteedNotToBeUndefOrPoison instead? |
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
---|---|---|
7238 | I was confusing the undef and poison value. Thanks for your example. |
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
---|---|---|
7211 | Locally I "fixed" this for AANonNull and AADereferenceable already didn't find the time to upstream yet. We want to move the followUsesInMBEC into the initialize of the floating subclass. We then make the argument and call site return subclass derive from the floating one though. |
BuildLibCalls.cpp 's inferLibFuncAttributes seems to be the right place for this?
I'll start with adding noundef to return values of library functions.
I think it is safer to gradually add noundef to arguments of library functions, since it may cause end-to-end miscompilation (if there exists any incorrect transformation): IIRC there was a regression from strlen having nonnull attribute at its pointer argument, that happened from its interaction with llvm.assume.
That would be great, and yest that is the right place for most of it IIRC.
I think it is safer to
Safer than what?
gradually add noundef to arguments of library functions, since it may cause end-to-end miscompilation (if there exists any incorrect transformation): IIRC there was a regression from strlen having nonnull attribute at its pointer argument, that happened from its interaction with llvm.assume.
I wouldn't be totally surprised if something explodes but it should not and it is good to know earlier if it does ;)
I made D85345.
Rather than adding noundef to all library functions' return values, I chose standard I/O functions and added noundef to its return values / arguments instead.
The reason is that some functions did not seem to be clear whether its return value is noundef.
memcmp is one example. (load p) - (load q) can be folded into memcmp(p, q, bytesz), but the former expression can be poison or undef whereas the latter one raises UB in that case.
I think it is safer to
Safer than what?
(than adding noundef to all library functions' args :)
gradually add noundef to arguments of library functions, since it may cause end-to-end miscompilation (if there exists any incorrect transformation): IIRC there was a regression from strlen having nonnull attribute at its pointer argument, that happened from its interaction with llvm.assume.
I wouldn't be totally surprised if something explodes but it should not and it is good to know earlier if it does ;)
Yes, me too.
I saw, cool!
Rather than adding noundef to all library functions' return values, I chose standard I/O functions and added noundef to its return values / arguments instead.
The reason is that some functions did not seem to be clear whether its return value is noundef.
memcmp is one example. (load p) - (load q) can be folded into memcmp(p, q, bytesz), but the former expression can be poison or undef whereas the latter one raises UB in that case.
I don't think all of them should have noundef everywhere but some. The return of malloc and the argument of free were my examples. I/O as a start is good too.
(than adding noundef to all library functions' args :)
I never tried to say "all" ;)
@okura there is one comment wrt. the MBEC context that needs to be addressed, I think the rest is fine.
Sorry for the late response.
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
---|---|---|
7211 | Is the same true for AAAlign? |
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
---|---|---|
7211 | Yes. I'll fix them. |
One last question. Shouldn't we use isGuaranteedNotToBeUndefOrPoison in the initialization on the associated value?
Unsure but this example might be impacted:
void unknown(); void bar(int*); void foo() { int x; unknown(); bar(&x /* <- should be noundef as far as I can tell */); }
I think we can deduce noundef more strongly by using isGuaranteedNotToBeUndefOrPoison in the initialization.
But I'm not sure whether we "should" do or not.
In your example, I understand a pointer of an allocated variable should be noundef, but I don't understand why unknown function is needed and important.
Just to make sure we don't deduce noundef for the alloca based on the must-be-executed-context. I haven't tried this but when we run this with the patch, is there a noundef for the call site argument? You need to add -attributor-annotate-decl-cs as well to make sure we don't just skip it. I would expect there is none because we never initialized the alloca as noundef. If we did, please elaborate how so I understand ;)
I'm sorry for the late reply. I forgot to upload an updated patch.
I haven't tried this but when we run this with the patch, is there a noundef for the call site argument?
There is not noundef for the call site argument as you expected. And I also confirmed that using isGuaranteedNotToBeUndefOrPoison in initialize makes it possible to deduce noundef for the call site position.
I added your test in noundef.ll
Sorry, I haven't yet and I will. I rebase and fix conflict now. I will upload it soon.
Locally I "fixed" this for AANonNull and AADereferenceable already didn't find the time to upstream yet. We want to move the followUsesInMBEC into the initialize of the floating subclass. We then make the argument and call site return subclass derive from the floating one though.