Normalize usage of StrBoolAttr

[serge-sans-paille]

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.

[jdoerfert]

I like this change.

[nikic]

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]

clang/lib/CodeGen/CGCall.cpp

1819 ↗

(On Diff #332289)

Why did this one not change?

[serge-sans-paille]

In D99080#2642019, @nikic wrote:

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

[serge-sans-paille]

Add AutoUpgrade path, plus some extra asserts

[nikic]

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.

[nikic]

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.

[serge-sans-paille]

Fix missing include + add auto upgrade test

[serge-sans-paille]

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.

[nikic]

In D99080#2647980, @serge-sans-paille wrote:

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

[serge-sans-paille]

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

[nikic]

Should this one be abandoned now?

[serge-sans-paille]

In D99080#2723694, @nikic wrote:

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]

In D99080#2724930, @serge-sans-paille wrote:

In D99080#2723694, @nikic wrote:

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?

[spatel]

In D99080#2756453, @nikic wrote:

In D99080#2724930, @serge-sans-paille wrote:

In D99080#2723694, @nikic wrote:

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

[serge-sans-paille]

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.

[serge-sans-paille]

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''.

[serge-sans-paille]

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

[nikic]

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.

[nikic]

This patch can probably be abandoned?