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

[craig.topper]

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.

[efriedma]

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.

[jdoerfert]

In D86744#2243395, @efriedma wrote:

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.

[dblaikie]

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?

[craig.topper]

Rename to isValid and switch the polarity.

[dblaikie]

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

404–405

This and below might be more legibly written as:

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

now?

[craig.topper]

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

[craig.topper]

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]

llvm/lib/CodeGen/XRayInstrumentation.cpp

148

Ah, yeah, makes sense

llvm/include/llvm/IR/Attributes.h

public:
 
   /// Return true if the attribute is a string (target-dependent)
   /// attribute.
   bool isStringAttribute() const;
 
   /// Return true if the attribute is a type attribute.
   bool isTypeAttribute() const;
 
+  /// Return true if the attribute is the None AttrKind.
+  bool isNone() const { return !pImpl; }
+
   /// Return true if the attribute is present.
   bool hasAttribute(AttrKind Val) const;
 
   /// Return true if the target-dependent attribute is present.
   bool hasAttribute(StringRef Val) const;
 
   /// Return the attribute's kind as an enum (Attribute::AttrKind). This
   /// requires the attribute to be an enum or integer attribute.

llvm/lib/CodeGen/XRayInstrumentation.cpp

for (auto &T : MBB.terminators()) {
         BuildMI(MBB, T, T.getDebugLoc(), TII->get(Opc));
       }
     }
 }
 
 bool XRayInstrumentation::runOnMachineFunction(MachineFunction &MF) {
   auto &F = MF.getFunction();
   auto InstrAttr = F.getFnAttribute("function-instrument");
-  bool AlwaysInstrument = !InstrAttr.hasAttribute(Attribute::None) &&
+  bool AlwaysInstrument = !InstrAttr.isNone() &&
                           InstrAttr.isStringAttribute() &&
                           InstrAttr.getValueAsString() == "xray-always";
   auto ThresholdAttr = F.getFnAttribute("xray-instruction-threshold");
   auto IgnoreLoopsAttr = F.getFnAttribute("xray-ignore-loops");
   unsigned int XRayThreshold = 0;
   if (!AlwaysInstrument) {
-    if (ThresholdAttr.hasAttribute(Attribute::None) ||
+    if (ThresholdAttr.isNone() ||
         !ThresholdAttr.isStringAttribute())
       return false; // XRay threshold attribute not found.
     if (ThresholdAttr.getValueAsString().getAsInteger(10, XRayThreshold))
       return false; // Invalid value for threshold.
 
-    bool IgnoreLoops = !IgnoreLoopsAttr.hasAttribute(Attribute::None);
+    bool IgnoreLoops = !IgnoreLoopsAttr.isNone();
 
     // Count the number of MachineInstr`s in MachineFunction
     int64_t MICount = 0;
     for (const auto &MBB : MF)
       MICount += MBB.size();
 
     bool TooFewInstrs = MICount < XRayThreshold;
 

llvm/lib/IR/Attributes.cpp

AttributeList::get(LLVMContext &C,
   assert(llvm::is_sorted(Attrs,
                          [](const std::pair<unsigned, Attribute> &LHS,
                             const std::pair<unsigned, Attribute> &RHS) {
                            return LHS.first < RHS.first;
                          }) &&
          "Misordered Attributes list!");
   assert(llvm::none_of(Attrs,
                        [](const std::pair<unsigned, Attribute> &Pair) {
-                         return Pair.second.hasAttribute(Attribute::None);
+                         return Pair.second.isNone();
                        }) &&
          "Pointless attribute!");
 
   // Create a vector if (unsigned, AttributeSetNode*) pairs from the attributes
   // list.
   SmallVector<std::pair<unsigned, AttributeSet>, 8> AttrPairVec;
   for (ArrayRef<std::pair<unsigned, Attribute>>::iterator I = Attrs.begin(),
          E = Attrs.end(); I != E; ) {

llvm/lib/Target/AArch64/AArch64TargetMachine.cpp

 
 AArch64TargetMachine::~AArch64TargetMachine() = default;
 
 const AArch64Subtarget *
 AArch64TargetMachine::getSubtargetImpl(const Function &F) const {
   Attribute CPUAttr = F.getFnAttribute("target-cpu");
   Attribute FSAttr = F.getFnAttribute("target-features");
 
-  std::string CPU = !CPUAttr.hasAttribute(Attribute::None)
+  std::string CPU = !CPUAttr.isNone()
                         ? CPUAttr.getValueAsString().str()
                         : TargetCPU;
-  std::string FS = !FSAttr.hasAttribute(Attribute::None)
+  std::string FS = !FSAttr.isNone()
                        ? FSAttr.getValueAsString().str()
                        : TargetFS;
 
   auto &I = SubtargetMap[CPU + FS];
   if (!I) {
     // This needs to be done before we create a new subtarget since any
     // creation will depend on the TM and the code generation flags on the
     // function that reside in TargetOptions.

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

 bool AMDGPUTargetMachine::EnableLateStructurizeCFG = false;
 bool AMDGPUTargetMachine::EnableFunctionCalls = false;
 bool AMDGPUTargetMachine::EnableFixedFunctionABI = false;
 
 AMDGPUTargetMachine::~AMDGPUTargetMachine() = default;
 
 StringRef AMDGPUTargetMachine::getGPUName(const Function &F) const {
   Attribute GPUAttr = F.getFnAttribute("target-cpu");
-  return GPUAttr.hasAttribute(Attribute::None) ?
+  return GPUAttr.isNone() ?
     getTargetCPU() : GPUAttr.getValueAsString();
 }
 
 StringRef AMDGPUTargetMachine::getFeatureString(const Function &F) const {
   Attribute FSAttr = F.getFnAttribute("target-features");
 
-  return FSAttr.hasAttribute(Attribute::None) ?
+  return FSAttr.isNone() ?
     getTargetFeatureString() :
     FSAttr.getValueAsString();
 }
 
 /// Predicate for Internalize pass.
 static bool mustPreserveGV(const GlobalValue &GV) {
   if (const Function *F = dyn_cast<Function>(&GV))
     return F->isDeclaration() || AMDGPU::isEntryFunctionCC(F->getCallingConv());

llvm/lib/Target/ARM/ARMTargetMachine.cpp

 
 ARMBaseTargetMachine::~ARMBaseTargetMachine() = default;
 
 const ARMSubtarget *
 ARMBaseTargetMachine::getSubtargetImpl(const Function &F) const {
   Attribute CPUAttr = F.getFnAttribute("target-cpu");
   Attribute FSAttr = F.getFnAttribute("target-features");
 
-  std::string CPU = !CPUAttr.hasAttribute(Attribute::None)
+  std::string CPU = !CPUAttr.isNone()
                         ? CPUAttr.getValueAsString().str()
                         : TargetCPU;
-  std::string FS = !FSAttr.hasAttribute(Attribute::None)
+  std::string FS = !FSAttr.isNone()
                        ? FSAttr.getValueAsString().str()
                        : TargetFS;
 
   // FIXME: This is related to the code below to reset the target options,
   // we need to know whether or not the soft float flag is set on the
   // function before we can generate a subtarget. We also need to use
   // it as a key for the subtarget since that can be the only difference
   // between two functions.

llvm/lib/Target/Hexagon/HexagonTargetMachine.cpp

 const HexagonSubtarget *
 HexagonTargetMachine::getSubtargetImpl(const Function &F) const {
   AttributeList FnAttrs = F.getAttributes();
   Attribute CPUAttr =
       FnAttrs.getAttribute(AttributeList::FunctionIndex, "target-cpu");
   Attribute FSAttr =
       FnAttrs.getAttribute(AttributeList::FunctionIndex, "target-features");
 
-  std::string CPU = !CPUAttr.hasAttribute(Attribute::None)
+  std::string CPU = !CPUAttr.isNone()
                         ? CPUAttr.getValueAsString().str()
                         : TargetCPU;
-  std::string FS = !FSAttr.hasAttribute(Attribute::None)
+  std::string FS = !FSAttr.isNone()
                        ? FSAttr.getValueAsString().str()
                        : TargetFS;
   // Append the preexisting target features last, so that +mattr overrides
   // the "unsafe-fp-math" function attribute.
   // Creating a separate target feature is not strictly necessary, it only
   // exists to make "unsafe-fp-math" force creating a new subtarget.
 
   if (FnAttrs.hasFnAttribute("unsafe-fp-math") &&

llvm/lib/Target/Mips/MipsTargetMachine.cpp

MipselTargetMachine::MipselTargetMachine(const Target &T, const Triple &TT,
                                          CodeGenOpt::Level OL, bool JIT)
     : MipsTargetMachine(T, TT, CPU, FS, Options, RM, CM, OL, JIT, true) {}
 
 const MipsSubtarget *
 MipsTargetMachine::getSubtargetImpl(const Function &F) const {
   Attribute CPUAttr = F.getFnAttribute("target-cpu");
   Attribute FSAttr = F.getFnAttribute("target-features");
 
-  std::string CPU = !CPUAttr.hasAttribute(Attribute::None)
+  std::string CPU = !CPUAttr.isNone()
                         ? CPUAttr.getValueAsString().str()
                         : TargetCPU;
-  std::string FS = !FSAttr.hasAttribute(Attribute::None)
+  std::string FS = !FSAttr.isNone()
                        ? FSAttr.getValueAsString().str()
                        : TargetFS;
   bool hasMips16Attr =
-      !F.getFnAttribute("mips16").hasAttribute(Attribute::None);
+      !F.getFnAttribute("mips16").isNone();
   bool hasNoMips16Attr =
-      !F.getFnAttribute("nomips16").hasAttribute(Attribute::None);
+      !F.getFnAttribute("nomips16").isNone();
 
   bool HasMicroMipsAttr =
-      !F.getFnAttribute("micromips").hasAttribute(Attribute::None);
+      !F.getFnAttribute("micromips").isNone();
   bool HasNoMicroMipsAttr =
-      !F.getFnAttribute("nomicromips").hasAttribute(Attribute::None);
+      !F.getFnAttribute("nomicromips").isNone();
 
   // FIXME: This is related to the code below to reset the target options,
   // we need to know whether or not the soft float flag is set on the
   // function, so we can enable it as a subtarget feature.
   bool softFloat =
       F.hasFnAttribute("use-soft-float") &&
       F.getFnAttribute("use-soft-float").getValueAsString() == "true";
 

llvm/lib/Target/PowerPC/PPCTargetMachine.cpp

 
 PPCTargetMachine::~PPCTargetMachine() = default;
 
 const PPCSubtarget *
 PPCTargetMachine::getSubtargetImpl(const Function &F) const {
   Attribute CPUAttr = F.getFnAttribute("target-cpu");
   Attribute FSAttr = F.getFnAttribute("target-features");
 
-  std::string CPU = !CPUAttr.hasAttribute(Attribute::None)
+  std::string CPU = !CPUAttr.isNone()
                         ? CPUAttr.getValueAsString().str()
                         : TargetCPU;
-  std::string FS = !FSAttr.hasAttribute(Attribute::None)
+  std::string FS = !FSAttr.isNone()
                        ? FSAttr.getValueAsString().str()
                        : TargetFS;
 
   // FIXME: This is related to the code below to reset the target options,
   // we need to know whether or not the soft float flag is set on the
   // function before we can generate a subtarget. We also need to use
   // it as a key for the subtarget since that can be the only difference
   // between two functions.

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp

RISCVTargetMachine::RISCVTargetMachine(const Target &T, const Triple &TT,
   setMachineOutliner(true);
 }
 
 const RISCVSubtarget *
 RISCVTargetMachine::getSubtargetImpl(const Function &F) const {
   Attribute CPUAttr = F.getFnAttribute("target-cpu");
   Attribute FSAttr = F.getFnAttribute("target-features");
 
-  std::string CPU = !CPUAttr.hasAttribute(Attribute::None)
+  std::string CPU = !CPUAttr.isNone()
                         ? CPUAttr.getValueAsString().str()
                         : TargetCPU;
-  std::string FS = !FSAttr.hasAttribute(Attribute::None)
+  std::string FS = !FSAttr.isNone()
                        ? FSAttr.getValueAsString().str()
                        : TargetFS;
   std::string Key = CPU + FS;
   auto &I = SubtargetMap[Key];
   if (!I) {
     // This needs to be done before we create a new subtarget since any
     // creation will depend on the TM and the code generation flags on the
     // function that reside in TargetOptions.

llvm/lib/Target/Sparc/SparcTargetMachine.cpp

 
 SparcTargetMachine::~SparcTargetMachine() {}
 
 const SparcSubtarget *
 SparcTargetMachine::getSubtargetImpl(const Function &F) const {
   Attribute CPUAttr = F.getFnAttribute("target-cpu");
   Attribute FSAttr = F.getFnAttribute("target-features");
 
-  std::string CPU = !CPUAttr.hasAttribute(Attribute::None)
+  std::string CPU = !CPUAttr.isNone()
                         ? CPUAttr.getValueAsString().str()
                         : TargetCPU;
-  std::string FS = !FSAttr.hasAttribute(Attribute::None)
+  std::string FS = !FSAttr.isNone()
                        ? FSAttr.getValueAsString().str()
                        : TargetFS;
 
   // FIXME: This is related to the code below to reset the target options,
   // we need to know whether or not the soft float flag is set on the
   // function, so we can enable it as a subtarget feature.
   bool softFloat =
       F.hasFnAttribute("use-soft-float") &&

llvm/lib/Target/SystemZ/SystemZTargetMachine.cpp

 
 SystemZTargetMachine::~SystemZTargetMachine() = default;
 
 const SystemZSubtarget *
 SystemZTargetMachine::getSubtargetImpl(const Function &F) const {
   Attribute CPUAttr = F.getFnAttribute("target-cpu");
   Attribute FSAttr = F.getFnAttribute("target-features");
 
-  std::string CPU = !CPUAttr.hasAttribute(Attribute::None)
+  std::string CPU = !CPUAttr.isNone()
                         ? CPUAttr.getValueAsString().str()
                         : TargetCPU;
-  std::string FS = !FSAttr.hasAttribute(Attribute::None)
+  std::string FS = !FSAttr.isNone()
                        ? FSAttr.getValueAsString().str()
                        : TargetFS;
 
   // FIXME: This is related to the code below to reset the target options,
   // we need to know whether or not the soft float flag is set on the
   // function, so we can enable it as a subtarget feature.
   bool softFloat =
     F.hasFnAttribute("use-soft-float") &&

llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp

WebAssemblyTargetMachine::getSubtargetImpl(std::string CPU,
   return I.get();
 }
 
 const WebAssemblySubtarget *
 WebAssemblyTargetMachine::getSubtargetImpl(const Function &F) const {
   Attribute CPUAttr = F.getFnAttribute("target-cpu");
   Attribute FSAttr = F.getFnAttribute("target-features");
 
-  std::string CPU = !CPUAttr.hasAttribute(Attribute::None)
+  std::string CPU = !CPUAttr.isNone()
                         ? CPUAttr.getValueAsString().str()
                         : TargetCPU;
-  std::string FS = !FSAttr.hasAttribute(Attribute::None)
+  std::string FS = !FSAttr.isNone()
                        ? FSAttr.getValueAsString().str()
                        : TargetFS;
 
   // This needs to be done before we create a new subtarget since any
   // creation will depend on the TM and the code generation flags on the
   // function that reside in TargetOptions.
   resetTargetOptions(F);
 

llvm/lib/Target/X86/X86TargetMachine.cpp

 X86TargetMachine::~X86TargetMachine() = default;
 
 const X86Subtarget *
 X86TargetMachine::getSubtargetImpl(const Function &F) const {
   Attribute CPUAttr = F.getFnAttribute("target-cpu");
   Attribute TuneAttr = F.getFnAttribute("tune-cpu");
   Attribute FSAttr = F.getFnAttribute("target-features");
 
-  StringRef CPU = !CPUAttr.hasAttribute(Attribute::None)
+  StringRef CPU = !CPUAttr.isNone()
                       ? CPUAttr.getValueAsString()
                       : (StringRef)TargetCPU;
-  StringRef TuneCPU = !TuneAttr.hasAttribute(Attribute::None)
+  StringRef TuneCPU = !TuneAttr.isNone()
                       ? TuneAttr.getValueAsString()
                       : (StringRef)CPU;
-  StringRef FS = !FSAttr.hasAttribute(Attribute::None)
+  StringRef FS = !FSAttr.isNone()
                      ? FSAttr.getValueAsString()
                      : (StringRef)TargetFS;
 
   SmallString<512> Key;
   // The additions here are ordered so that the definitely short strings are
   // added first so we won't exceed the small size. We append the
   // much longer FS string at the end so that we only heap allocate at most
   // one time.
 
   // Extract prefer-vector-width attribute.
   unsigned PreferVectorWidthOverride = 0;
   Attribute PreferVecWidthAttr = F.getFnAttribute("prefer-vector-width");
-  if (!PreferVecWidthAttr.hasAttribute(Attribute::None)) {
+  if (!PreferVecWidthAttr.isNone()) {
     StringRef Val = PreferVecWidthAttr.getValueAsString();
     unsigned Width;
     if (!Val.getAsInteger(0, Width)) {
       Key += "prefer-vector-width=";
       Key += Val;
       PreferVectorWidthOverride = Width;
     }
   }
 
   // Extract min-legal-vector-width attribute.
   unsigned RequiredVectorWidth = UINT32_MAX;
   Attribute MinLegalVecWidthAttr = F.getFnAttribute("min-legal-vector-width");
-  if (!MinLegalVecWidthAttr.hasAttribute(Attribute::None)) {
+  if (!MinLegalVecWidthAttr.isNone()) {
     StringRef Val = MinLegalVecWidthAttr.getValueAsString();
     unsigned Width;
     if (!Val.getAsInteger(0, Width)) {
       Key += "min-legal-vector-width=";
       Key += Val;
       RequiredVectorWidth = Width;
     }
   }

llvm/lib/Transforms/IPO/LowerTypeTests.cpp

Constant *Target = ConstantExpr::getSelect(
                             Constant::getNullValue(F->getType())),
       JT, Constant::getNullValue(F->getType()));
   PlaceholderFn->replaceAllUsesWith(Target);
   PlaceholderFn->eraseFromParent();
 }
 
 static bool isThumbFunction(Function *F, Triple::ArchType ModuleArch) {
   Attribute TFAttr = F->getFnAttribute("target-features");
-  if (!TFAttr.hasAttribute(Attribute::None)) {
+  if (!TFAttr.isNone()) {
     SmallVector<StringRef, 6> Features;
     TFAttr.getValueAsString().split(Features, ',');
     for (StringRef Feature : Features) {
       if (Feature == "-thumb-mode")
         return false;
       else if (Feature == "+thumb-mode")
         return true;
     }

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp

if (CSInfo.isExported())
       IsExported = true;
     if (CSInfo.AllCallSitesDevirted)
       return;
     for (auto &&VCallSite : CSInfo.CallSites) {
       CallBase &CB = VCallSite.CB;
 
       // Jump tables are only profitable if the retpoline mitigation is enabled.
       Attribute FSAttr = CB.getCaller()->getFnAttribute("target-features");
-      if (FSAttr.hasAttribute(Attribute::None) ||
-          !FSAttr.getValueAsString().contains("+retpoline"))
+      if (FSAttr.isNone() || !FSAttr.getValueAsString().contains("+retpoline"))
         continue;
 
       if (RemarksEnabled)
         VCallSite.emitRemark("branch-funnel",
                              JT->stripPointerCasts()->getName(), OREGetter);
 
       // Pass the address of the vtable in the nest register, which is r10 on
       // x86_64.