Page MenuHomePhabricator

[DAE] Adjust param/arg attributes when changing parameter to undef
ClosedPublic

Authored by Carrot on Mar 18 2021, 2:29 PM.

Details

Summary

In DeadArgumentElimination pass, if a function's argument is never used, corresponding caller's parameter can be changed to undef. If the param/arg has attribute noundef or other related attributes, LLVM LangRef(https://llvm.org/docs/LangRef.html#parameter-attributes) says its behavior is undefined. SimplifyCFG(D97244) takes advantage of this behavior and does bad transformation on valid code.

To avoid this undefined behavior when change caller's parameter to undef, this patch removes noundef attribute and other attributes imply noundef on param/arg.

Diff Detail

Event Timeline

Carrot created this revision.Mar 18 2021, 2:29 PM
Carrot requested review of this revision.Mar 18 2021, 2:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2021, 2:29 PM

Adding potential reviewers. I know there are some analysis helpers for this sort of thing, but not sure if those work for this usage.

nikic added a comment.Mar 21 2021, 9:15 AM

This looks reasonable to me. Dropping nonnull is not strictly necessary (because it has no immediate UB), but doesn't hurt either.

I was going to suggest using AttrBuilder to drop all attributes at once, but I'm not sure that works with dereferenceable attributes.

jdoerfert added inline comments.Mar 21 2021, 9:51 AM
llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
323

There should be a helper that populates a list of attributes we should drop when transforming something to undef.

This looks reasonable to me. Dropping nonnull is not strictly necessary (because it has no immediate UB), but doesn't hurt either.

I was going to suggest using AttrBuilder to drop all attributes at once, but I'm not sure that works with dereferenceable attributes.

Thanks for the good suggestion.
According to the implementation of AttrBuilder::remove, it can remove dereferenceable attribute from another AttrBuilder with any dereferenceable bytes.

Carrot updated this revision to Diff 332495.Mar 22 2021, 6:40 PM
Carrot marked an inline comment as done.
Carrot updated this revision to Diff 332803.Mar 23 2021, 3:20 PM

Reformat the patch.

jdoerfert accepted this revision.Mar 23 2021, 9:39 PM

LGTM with two nits. Others can chime in if they dislike the wording.

llvm/include/llvm/IR/Attributes.h
517–518 ↗(On Diff #332803)

I'd make this more explicit, here and below.

520 ↗(On Diff #332803)

Again, more explicit what this does, here and below.

This revision is now accepted and ready to land.Mar 23 2021, 9:39 PM
Carrot updated this revision to Diff 333070.Mar 24 2021, 11:33 AM
Carrot marked 2 inline comments as done.

Thanks for the improvement.
Will commit this version.