Page MenuHomePhabricator

Normalize usage of StrBoolAttr
Needs ReviewPublic

Authored by serge-sans-paille on Mar 22 2021, 8:13 AM.

Details

Reviewers
nikic
Summary

Priori to this patch, StrBoolAttr attributes had the strange property to be either unset, set to false or set to true. And the associated value was a string containing either "true" or "false". This is both cumbersome to use and inefficient.

This patch propose to no longer use the value filed, and just check the presence of the attribute, which was already an established practice in part of the LLVM codebase, see for instance https://reviews.llvm.org/D96400
As a side effect, when compiling sqlite3.c source code in -O0, this change makes the instruction count decrease from 6,157,211,048 to 6,155,407,798. Simpler and faster.

Diff Detail

Event Timeline

serge-sans-paille requested review of this revision.Mar 22 2021, 8:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2021, 8:13 AM

I like this change.

Looks good conceptually. Only concern I have is that we used to emit some of these attributes with explicit ="false" (I think -- correct me if I'm wrong), in which case existing bitcode would now be interpreted as if the attribute was enabled, not disabled. I think we need some AutoUpgrade handling to drop the attribute from existing bitcode in that case.

nikic added inline comments.Mar 22 2021, 10:57 AM
clang/lib/CodeGen/CGCall.cpp
1819

Why did this one not change?

Looks good conceptually. Only concern I have is that we used to emit some of these attributes with explicit ="false" (I think -- correct me if I'm wrong), in which case existing bitcode would now be interpreted as if the attribute was enabled, not disabled. I think we need some AutoUpgrade handling to drop the attribute from existing bitcode in that case.

OK, I never did this before but that definitively sounds important to me.

clang/lib/CodeGen/CGCall.cpp
1819

Because it's not listed as a`StrBoolAttr` but just a regular String Attribute

Add AutoUpgrade path, plus some extra asserts

nikic accepted this revision.Mar 23 2021, 1:00 PM

LGTM apart from the broken assert.

llvm/lib/IR/Attributes.cpp
1672

This needs a llvm/IR/Attributes.inc include to actually do anything. Also typo doens't.

This revision is now accepted and ready to land.Mar 23 2021, 1:00 PM
nikic requested changes to this revision.Mar 23 2021, 1:02 PM

On second thought, I think this needs a test for the AutoUpgrade part. In particular I suspect that the AutoUpgrade will not work if you actually enable that assertion you added, as that also goes through AttrBuilder.

This revision now requires changes to proceed.Mar 23 2021, 1:02 PM

Fix missing include + add auto upgrade test

Thanks @nikic for pointing me in the assert direction; i've been prting without much trouble all exisiting test to use the form proposed in this review, and I'm now in an interesting situation; What if the following code:

 define double @unsafe_fp_math_off(double %x) {
  %div = fdiv double %x, 2.0
  ret double %div
}

gets compiled without extra flag? There's no unsafe math flag anywhere, so it's indeed a div. If I add the attribute "unsafe-fp-math" I get a mul, and that's fine.

If I compiler this code with -enable-unsafe-fp-math from llc, it's equivalent to having the "unsafe-fp-math", and that's fine too. But then with current design, there's no way to *force* that function into safe mode, whatever the CLI passed to llc.

Stated otherwise, having three states for a str bool attribute indeed has one quality: the attribute can be in the *unset* state, where CLI argument can prevail.

If I compiler this code with -enable-unsafe-fp-math from llc, it's equivalent to having the "unsafe-fp-math", and that's fine too. But then with current design, there's no way to *force* that function into safe mode, whatever the CLI passed to llc.

Stated otherwise, having three states for a str bool attribute indeed has one quality: the attribute can be in the *unset* state, where CLI argument can prevail.

My view on this is that all relevant information must be encoded in the IR (be it via FMF flags, attributes or module flags) for LTO reasons. opt/llc flags can only exist as a testing convenience, and it's okay if they force behavior without a way to opt-out. If you need to opt-out, don't pass the flag :)

I kept the simplifcation of the API while not chagnign the representation int his alternative review: https://reviews.llvm.org/D99299 It doesn't have the performance gain of this patch, but the representation is unchanged which makes it easier to review. And it's backward compatible, so no need to auto-upgrade

Should this one be abandoned now?

Should this one be abandoned now?

It depends. I'd like it to land, but that means changing the behavior of

--enable-no-infs-fp-math                                              - Enable FP math optimizations that assume no +-Infs
--enable-no-nans-fp-math                                              - Enable FP math optimizations that assume no NaNs
--enable-no-signed-zeros-fp-math                                      - Enable FP math optimizations that assume the sign of 0 is insignificant
--enable-no-trapping-fp-math                                          - Enable setting the FP exceptions build attribute not to use exceptions
--enable-unsafe-fp-math                                               - Enable optimizations that may decrease FP precision

from lli. Maybe I should post a small RFC on that topic ?