Page MenuHomePhabricator

[Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default
ClosedPublic

Authored by hyeongyukim on Jun 29 2021, 9:37 PM.

Details

Summary

Turning on enable_noundef_analysis flag allows better codegen by removing freeze instructions.
I modified clang by renaming enable_noundef_analysis flag to disable-noundef-analysis and turning it off by default.

Test updates are made as a separate patch: D108453

Diff Detail

Event Timeline

hyeongyukim created this revision.Jun 29 2021, 9:37 PM
hyeongyukim requested review of this revision.Jun 29 2021, 9:37 PM
hyeongyukim edited the summary of this revision. (Show Details)

A large amount of testing will be affected, and I will update it soon.

Changed disable_noundef_args flag to enable_noundef_args to enable emitting noundef attributes on IR call arguments and return values by default.
Changed test codes will be attached at another PR.

hyeongyukim retitled this revision from [Clang/Test]: Enable enable_noundef_analysis as default to [Clang/Test]: Enable enable_noundef_analysis by default.Aug 20 2021, 3:01 AM
hyeongyukim edited the summary of this revision. (Show Details)
aqjune updated this revision to Diff 368033.Aug 22 2021, 10:38 PM

Rename disable-noundef-args to disable-noundef-analysis

aqjune retitled this revision from [Clang/Test]: Enable enable_noundef_analysis by default to [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default.
aqjune edited the summary of this revision. (Show Details)

Hello all,

I'm a messenger of @hyeongyukim's two patches that allow clang to turn on noundef analysis by default.
The noundef analysis flag was added by D81678 in the past. Its goal is to mark arguments and return values in C/C++ as noundef if legal.
Attaching noundef is beneficial because it allows quite a few optimizations that are unsound w.r.t. undef or poison (they are usually guarded with isGuaranteedNotToBeUndefOrPoison check).
Since the fact that arg/ret values are noundef is derived from the source language (C/C++)'s specification, the information is permanently lost unless explicitly attached by clang.

Previously, the flag was not activated by default because it required a lot of tests to be updated, possibly raising conflicts with downstream patches (discussed in this thread: D82317)
To handle the conflict, I'd like to update requested tests to simply adding -disable-noundef-analysis at // RUN: %clang_cc1 ... rather than fixing the whole text.
I'll talk about this more in the second patch (D108453).

kda added a subscriber: kda.Sep 1 2021, 3:14 PM
nlopes added a comment.Sep 7 2021, 2:37 AM

(repost my message from llvm-dev)

I can add one thought regarding why this direction makes sense and why it doesn't constrain optimizations.

Traditionally we don't want to mark too many things as UB as it restricts code movement and thus limits optimizations. That's why we have poison for e.g. signed arithmetic overflow rather than using UB as allowed by the C standard.
For function calls, however, optimizers are already super constrained: in general we cannot move them around because they may not return, they may throw an exception, they may modify memory, do a syscall, and so on. So adding another restriction to function calls isn't a big deal; optimizers don't touch them that much anyway.

This proposal adds more UB, as it turns undef/poison into UB on function calls, but that doesn't limit optimizations. So it seems like a good tradeoff: we learn some values can't be undef/poison "for free". Plus that UB can be dropped if needed; dropping noundef is legal! So there are really no downsides.
That's why I believe this is a good direction for clang.

From the users perspective, hopefully the sanitizers can already flag related issues so hopefully this won't lead to hard-to-debug UB bugs.

I did some experiments to confirm the benefits of adding a noundef attribute to the function parameter. (test result link)

One of the most significant advantages of this patch is that it can improve performance by removing unnecessary Freeze.
To check whether the performance is improved, first, I checked how much performance regression occurred on SPECrate2017 when we fixed the miscompilation problem of LoopUnswitch through the Freeze (D106041.)
Then, I applied this patch to see if the unnecessary Freeze was removed and performance was improved.
From the experimental results, the following facts were found.

  • Fixing only LoopUnswitch(D106041) reduces runtime performance and increases object size.
  • Freeze added that fixing the LoopUnswitch interferes with other optimizations, increasing the number of instructions. (There is still a problem with LoopUnswitch due to these performance problems)
  • Applying this patch improves runtime and makes the object size almost the same as the LLVM trunk.
  • Knowing that Function arguments are not noundef/poison, some freezes were removed, and performance and binary size have been improved.

The second sheet compares the performance of the LLVM trunk and this patch, but there is no noticeable change.
In the last sheet, about 2,000 LLVM test-suites were compiled, and the object size was compared. In most cases, the object size was improved.

I think many advantages can be obtained by applying this patch.

@hyeongyukim, thank you for the summary. This looks like a great change, and a net positive to me. The test churn in the other patch is unfortunate, but IMHO unavoidable.

If no one has any further objections,
LGTM

aqjune added a comment.Thu, Oct 7, 6:16 PM

Thank you!

I'll send a mail to cfe-dev + llvm-dev to notify the change (and gather any possible concern if exists).

If there is no objection in a week, let's land these patches.

Thank you!
In the meantime, I will rebase this patch and resolve conflicts.

There has been no concern in a week, so I'd like to land this patch and D108453 in this weekend.
I'll carefully watch the buildbots and address failures if any.
@eugenis could you accept this patch and D108453 please, if you don't mind?

eugenis accepted this revision.Fri, Oct 15, 9:54 AM
This revision is now accepted and ready to land.Fri, Oct 15, 9:54 AM