Page MenuHomePhabricator

Normalize interaction with boolean attributes
ClosedPublic

Authored by serge-sans-paille on Mar 24 2021, 2:02 PM.

Details

Summary

Such attributes can either be unset, or set to "true" or "false" (as string).
throughout the codebase, this led to inelegant checks ranging from

if (Fn->getFnAttribute("no-jump-tables").getValueAsString() == "true")

to

if (Fn->hasAttribute("no-jump-tables") && Fn->getFnAttribute("no-jump-tables").getValueAsString() == "true")

Introduce a getValueAsBool that normalize the check, with the following
behavior:

no attributes or attribute set to "false" => return false
attribute set to "true" => return true

Diff Detail

Event Timeline

serge-sans-paille requested review of this revision.Mar 24 2021, 2:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2021, 2:02 PM

@nikic up :-) Before I try to make that part more efficient, at least we now have a nicer interface.

nikic added a comment.Apr 7 2021, 1:39 PM

As you are now asserting that only certain values are allowed, I believe you also need to add a verifier check that checks this. It should not be possible to assert using a valid module.

What is the relation to D99080 here? Is this intended as preliminary cleanup for that change, or the final result? I quite liked the direction that one was going...

As you are now asserting that only certain values are allowed, I believe you also need to add a verifier check that checks this. It should not be possible to assert using a valid module.

Great Idea, will do!

What is the relation to D99080 here? Is this intended as preliminary cleanup for that change, or the final result? I quite liked the direction that one was going...

So was I, but I wanted to prevent any regression. So this is preliminary work on the API side, so that I can do internal change on the representation, without any user code change.
I will try changing the representation of the bool value from str to a real bool...

Add verifier step

nikic added inline comments.Apr 11 2021, 7:32 AM
llvm/lib/IR/Attributes.cpp
661

By the way, why is empty string allowed as a value here? Are there existing uses of empty string as false?

llvm/lib/IR/Verifier.cpp
1726

This should use CheckFailed rather than asserting.

Fix and test verifier

serge-sans-paille marked 2 inline comments as done.Tue, Apr 13, 1:22 PM
serge-sans-paille added inline comments.
llvm/lib/IR/Attributes.cpp
661

Because that's how an attribute is return when it's asked for while not being specified. This avoid having to check for the attribute presence before asking for its value. See Below:

if (FnAttrs.hasFnAttribute("unsafe-fp-math") &&
      F.getFnAttribute("unsafe-fp-math").getValueAsString() == "true")

becomes

if (F.getFnAttribute("unsafe-fp-math").getValueAsBool())
nikic accepted this revision.Tue, Apr 13, 1:25 PM

LGTM

llvm/lib/IR/Attributes.cpp
661

Ah yes, that makes sense.

This revision is now accepted and ready to land.Tue, Apr 13, 1:25 PM
This revision was landed with ongoing or failed builds.Fri, Apr 16, 11:17 PM
This revision was automatically updated to reflect the committed changes.
serge-sans-paille marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptFri, Apr 16, 11:17 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
dblaikie added inline comments.
llvm/lib/IR/Attributes.cpp
660–663

Is it maybe worth using named constants rather than string literals, so this true and false matches up with the true/false in StrBoolAttr? (I assume that's where these strings are produced?)

llvm/lib/IR/Attributes.cpp
660–663

Agreed. The whole strbool attribute stuff looks very inefficient to me! I'll investigate that aspect.