This is an archive of the discontinued LLVM Phabricator instance.

Normalize usage of StrBoolAttr
Needs ReviewPublic

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

Details

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 ↗(On Diff #332289)

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 ↗(On Diff #332289)

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 ?

nikic added a subscriber: spatel.May 13 2021, 3:17 AM

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 ?

It would be ideal to drop these llc flags completely, and adjust the tests that use them to use FMF flags. I don't think they serve any purpose apart from making bad tests simpler to write. @spatel Are there any nearer-term plans to drop support for global fast math attributes/flags?

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 ?

It would be ideal to drop these llc flags completely, and adjust the tests that use them to use FMF flags. I don't think they serve any purpose apart from making bad tests simpler to write.

Agree. IIUC, we can do that part now without waiting for the next question to be resolved...

@spatel Are there any nearer-term plans to drop support for global fast math attributes/flags?

We haven't made much progress lately. I think we need to propagate FMF on FP values more thoroughly -- function arguments, casts, and possibly memory ops (not sure how that reconciles with type-less memory) -- to guarantee that FMF is a complete replacement for the global flags in IR ( https://llvm.org/PR35607 ). But it's also possible that the backend is already good enough now that we can try it there (but it's a job that requires updating lots of codegen tests last time I checked).

It would be ideal to drop these llc flags completely, and adjust the tests that use them to use FMF flags. I don't think they serve any purpose apart from making bad tests simpler to write.

Agree. IIUC, we can do that part now without waiting for the next question to be resolved...

I'm going that way then.

This change introduces a behavior change: when an attribute is not set, it used to mean `use the default value''. It now means `attribute unset''.

Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a reviewer: gkm. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript

@nikic here is the follow-up you were looking for

nikic added a comment.May 22 2021, 6:14 AM

This is going to be pretty impossible to review in the current form :) I think it would be best if you just directly commit all changes that just do "remove "foobar"="false" from tests. That's just NFC cleanup of unnecessary attributes, and should cut down the size of this diff by a lot.

int3 resigned from this revision.Jun 3 2021, 6:42 PM
ormris removed a subscriber: ormris.Jun 7 2021, 4:11 PM
smeenai resigned from this revision.Apr 28 2022, 1:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2022, 1:58 PM
nikic resigned from this revision.Mar 7 2023, 1:31 AM

This patch can probably be abandoned?