This is an archive of the discontinued LLVM Phabricator instance.

[Attributor] Deduce noundef attribute
ClosedPublic

Authored by okura on Aug 4 2020, 1:24 AM.

Details

Summary

This patch introduces a new abstract attribute AANoUndef which corresponds to noundef IR attribute and deduce them.

Diff Detail

Event Timeline

okura created this revision.Aug 4 2020, 1:24 AM
Herald added a reviewer: homerdin. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
okura requested review of this revision.Aug 4 2020, 1:24 AM
aqjune added a subscriber: aqjune.Aug 4 2020, 1:29 AM
aqjune added inline comments.
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
7236

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?

okura added inline comments.Aug 4 2020, 1:44 AM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
7236

I was confusing the undef and poison value. Thanks for your example.
I will rewrite this part using isGuaranteedNotToBeUndefOrPoison.
I didn't know such a helpful function, thank you very much.

okura updated this revision to Diff 282861.Aug 4 2020, 4:23 AM
  • use isGuaranteedNotToBeUndefOrPoison in followUseInMBEC
  • fix comment
  • update tests
jdoerfert added inline comments.Aug 4 2020, 5:12 AM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
7209

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.

Forgot to say, this is really cool :) I'll take a closer look in a bit.

Note: @aqjune, and @okura We should also seed noundef in the intrinsics and especially known library functions definitions. I mean, malloc is not returning a undef or poison value (I hope), and free is not accepting one.

aqjune added a comment.Aug 4 2020, 9:55 PM

Note: @aqjune, and @okura We should also seed noundef in the intrinsics and especially known library functions definitions. I mean, malloc is not returning a undef or poison value (I hope), and free is not accepting one.

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.

Note: @aqjune, and @okura We should also seed noundef in the intrinsics and especially known library functions definitions. I mean, malloc is not returning a undef or poison value (I hope), and free is not accepting one.

BuildLibCalls.cpp 's inferLibFuncAttributes seems to be the right place for this?
I'll start with adding noundef to return values of library functions.

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 ;)

BuildLibCalls.cpp 's inferLibFuncAttributes seems to be the right place for this?
I'll start with adding noundef to return values of library functions.

That would be great, and yest that is the right place for most of it IIRC.

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 made D85345.

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.

okura added a comment.Aug 6 2020, 9:16 AM

Sorry for the late response.

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
7209

Is the same true for AAAlign?

okura updated this revision to Diff 283635.Aug 6 2020, 9:18 AM
  • call followUseInMBEC in AANoUndefFloating::initialize
jdoerfert added inline comments.Aug 7 2020, 1:39 PM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
7209

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 */);
}

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.

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 ;)

guiand added a subscriber: guiand.Aug 10 2020, 11:13 AM
okura added a comment.Aug 17 2020, 9:50 AM

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

I added your test in noundef.ll

you did not update the patch yet, right? (No rush, just to confirm you will)

I added your test in noundef.ll

you did not update the patch yet, right? (No rush, just to confirm you will)

Sorry, I haven't yet and I will. I rebase and fix conflict now. I will upload it soon.

okura updated this revision to Diff 286063.Aug 17 2020, 10:18 AM
  • use isGuaranteedNotUndefOrPoison in initialize
  • update tests
okura retitled this revision from [Attributor][WIP] Deduce noundef attribute to [Attributor] Deduce noundef attribute.Aug 17 2020, 10:19 AM
okura edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Aug 17 2020, 10:32 AM
okura updated this revision to Diff 286186.Aug 17 2020, 5:59 PM

This patch broke a test in Transforms/OpenMP/parallel_deletion.ll. I fixed the test.

okura updated this revision to Diff 286225.Aug 18 2020, 2:03 AM
  • fix conflict
This revision was automatically updated to reflect the committed changes.