This is an archive of the discontinued LLVM Phabricator instance.

[Attributes] Add a method to check if an Attribute has AttrKind None. Use instead of hasAttribute(Attribute::None)
ClosedPublic

Authored by craig.topper on Aug 27 2020, 3:32 PM.

Details

Summary

There's a special case in hasAttribute for None when pImpl is null. If pImpl is not null we dispatch to pImpl->hasAttribute which will always return false for Attribute::None.

So if we just want to check for None its sufficient to just check that pImpl is null. Which can even be done inline.

This patch adds a helper for that case which I hope will speed up our getSubtargetImpl implementations.

Alternatively we could extend the special case to only call pImpl->hasAttribute if we're not looking for None. But we couldn't do it inline without splitting hasAttribute into a inline and non-inline part.

Diff Detail

Event Timeline

craig.topper created this revision.Aug 27 2020, 3:32 PM
Herald added a project: Restricted Project. · View Herald Transcript
craig.topper requested review of this revision.Aug 27 2020, 3:32 PM

Maybe worth adding asserts to ensure we can't create an AttributeImpl for None?

Checking for isNone() is a little unintuitive; maybe something like hasAttribute() that checks for a non-None attribute would be better? As it is, it's a double-negative in most places it's used.

Maybe worth adding asserts to ensure we can't create an AttributeImpl for None?

Checking for isNone() is a little unintuitive; maybe something like hasAttribute() that checks for a non-None attribute would be better? As it is, it's a double-negative in most places it's used.

hasAttribute() or isAttribute() or either with AttrKind? Naming things is hard.

my 2c would be to, if possible, remove the "none" state & possibly use Optional to express it in return values.

Short of that - removing the API to query for it and using explicit operator bool. Or maybe operator bool and "isValid" or similarly named function?

Rename to isValid and switch the polarity.

dblaikie added inline comments.Aug 27 2020, 8:53 PM
llvm/lib/CodeGen/XRayInstrumentation.cpp
148

I'm guessing this was here (and below) as a fastpath? I assume you're removing it because it's probably premature optimization by your judgment? (I'm OK with that, just checking I understand the motivation)

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
403–404

This and below might be more legibly written as:

return GPUAttr.isValid() ? GpuAttr.getValueAsString() : getTargetCPU();

now?

-Flip the conditional order in AMDGPUTargetMachine.cpp
-Run clang-format

craig.topper added inline comments.Aug 27 2020, 9:33 PM
llvm/lib/CodeGen/XRayInstrumentation.cpp
148

I don't think it was much of a fastpath before. Neither hasAttribute or isStringAttribute are inline. If the pImpl pointer was non-null, hasAttribute would call hasAttribute on pImpl. I'm not sure that was any faster than calling isStringAttribute on pImpl. Neither method calls anything if pImpl is null.

Using isValid might be slightly faster now since its inlined, but seemed small.

dblaikie accepted this revision.Aug 28 2020, 12:55 PM
dblaikie added inline comments.
llvm/lib/CodeGen/XRayInstrumentation.cpp
148

Ah, yeah, makes sense

This revision is now accepted and ready to land.Aug 28 2020, 12:55 PM
This revision was landed with ongoing or failed builds.Aug 28 2020, 1:25 PM
This revision was automatically updated to reflect the committed changes.