[PATCH] D99299: Normalize interaction with boolean attributes

2021-04-22 Thread serge via Phabricator via cfe-commits
serge-sans-paille added inline comments.



Comment at: llvm/lib/IR/Attributes.cpp:660-663
+bool AttributeImpl::getValueAsBool() const {
+  assert(getValueAsString().empty() || getValueAsString() == "false" || 
getValueAsString() == "true");
+  return getValueAsString() == "true";
+}

dblaikie wrote:
> 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?)
> 
Agreed. The whole strbool attribute stuff looks very inefficient to me! I'll 
investigate that aspect.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99299/new/

https://reviews.llvm.org/D99299

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D99299: Normalize interaction with boolean attributes

2021-04-21 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added inline comments.



Comment at: llvm/lib/IR/Attributes.cpp:660-663
+bool AttributeImpl::getValueAsBool() const {
+  assert(getValueAsString().empty() || getValueAsString() == "false" || 
getValueAsString() == "true");
+  return getValueAsString() == "true";
+}

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



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99299/new/

https://reviews.llvm.org/D99299

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D99299: Normalize interaction with boolean attributes

2021-04-16 Thread serge via Phabricator via cfe-commits
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
serge-sans-paille marked an inline comment as done.
Closed by commit rGd6de1e1a7140: Normalize interaction with boolean attributes 
(authored by serge-sans-paille).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99299/new/

https://reviews.llvm.org/D99299

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  llvm/include/llvm/CodeGen/TargetLowering.h
  llvm/include/llvm/IR/Attributes.h
  llvm/lib/Analysis/IVDescriptors.cpp
  llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
  llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
  llvm/lib/IR/AttributeImpl.h
  llvm/lib/IR/Attributes.cpp
  llvm/lib/IR/Verifier.cpp
  llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp
  llvm/lib/Target/AMDGPU/AMDGPULibCalls.cpp
  llvm/lib/Target/AMDGPU/AMDGPULowerKernelAttributes.cpp
  llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.cpp
  llvm/lib/Target/ARM/ARMTargetMachine.cpp
  llvm/lib/Target/Hexagon/HexagonTargetMachine.cpp
  llvm/lib/Target/M68k/M68kISelLowering.cpp
  llvm/lib/Target/Mips/MipsTargetMachine.cpp
  llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp
  llvm/lib/Target/PowerPC/PPCTargetMachine.cpp
  llvm/lib/Target/Sparc/SparcTargetMachine.cpp
  llvm/lib/Target/SystemZ/SystemZTargetMachine.cpp
  llvm/lib/Target/TargetMachine.cpp
  llvm/lib/Target/X86/X86TargetMachine.cpp
  llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
  llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp
  llvm/lib/Transforms/Utils/SimplifyCFG.cpp
  llvm/test/Verifier/invalid-strbool-attr.ll

Index: llvm/test/Verifier/invalid-strbool-attr.ll
===
--- /dev/null
+++ llvm/test/Verifier/invalid-strbool-attr.ll
@@ -0,0 +1,9 @@
+; RUN: not llvm-as < %s -o /dev/null 2>&1 | FileCheck %s
+
+; CHECK: invalid value for 'no-jump-tables' attribute: yes
+
+define void @func() #0 {
+  ret void
+}
+
+attributes #0 = { "no-jump-tables"="yes" }
Index: llvm/lib/Transforms/Utils/SimplifyCFG.cpp
===
--- llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -5793,7 +5793,7 @@
   // Only build lookup table when we have a target that supports it or the
   // attribute is not set.
   if (!TTI.shouldBuildLookupTables() ||
-  (Fn->getFnAttribute("no-jump-tables").getValueAsString() == "true"))
+  (Fn->getFnAttribute("no-jump-tables").getValueAsBool()))
 return false;
 
   // FIXME: If the switch is too sparse for a lookup table, perhaps we could
Index: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp
===
--- llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp
+++ llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp
@@ -799,7 +799,7 @@
 AliasAnalysis *AA,
 OptimizationRemarkEmitter *ORE,
 DomTreeUpdater &DTU) {
-  if (F.getFnAttribute("disable-tail-calls").getValueAsString() == "true")
+  if (F.getFnAttribute("disable-tail-calls").getValueAsBool())
 return false;
 
   bool MadeChange = false;
Index: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
===
--- llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
+++ llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
@@ -5028,7 +5028,7 @@
   void visitCallBase(CallBase &CB, IRBuilder<> &IRB) override {
 bool IsSoftFloatABI = CB.getCalledFunction()
   ->getFnAttribute("use-soft-float")
-  .getValueAsString() == "true";
+  .getValueAsBool();
 unsigned GpOffset = SystemZGpOffset;
 unsigned FpOffset = SystemZFpOffset;
 unsigned VrIndex = 0;
Index: llvm/lib/Target/X86/X86TargetMachine.cpp
===
--- llvm/lib/Target/X86/X86TargetMachine.cpp
+++ llvm/lib/Target/X86/X86TargetMachine.cpp
@@ -294,8 +294,7 @@
   // 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.
-  bool SoftFloat =
-  F.getFnAttribute("use-soft-float").getValueAsString() == "true";
+  bool SoftFloat = F.getFnAttribute("use-soft-float").getValueAsBool();
   // If the soft float attribute is set on the function turn on the soft float
   // subtarget feature.
   if (SoftFloat)
Index: llvm/lib/Target/TargetMachine.cpp
===
--- llvm/lib/Target/TargetMachine.cpp
+++ llvm/lib/Target/TargetMachine.cpp
@@ -56,7 +56,7 @@
 void TargetMachine::resetTargetOptions(c