[PATCH] D103491: [ADT] Move DenseMapInfo for ArrayRef/StringRef into respective headers (NFC)

2021-06-03 Thread Nikita Popov via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG983565a6fe4a: [ADT] Move DenseMapInfo for ArrayRef/StringRef 
into respective headers (NFC) (authored by nikic).

Changed prior to commit:
  https://reviews.llvm.org/D103491?vs=349093=349588#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103491

Files:
  clang/include/clang/AST/ComparisonCategories.h
  llvm/include/llvm/ADT/ArrayRef.h
  llvm/include/llvm/ADT/DenseMapInfo.h
  llvm/include/llvm/ADT/StringRef.h
  llvm/include/llvm/IR/PassInstrumentation.h
  llvm/include/llvm/Support/Threading.h
  llvm/lib/CodeGen/AsmPrinter/WinException.h
  llvm/lib/CodeGen/MBFIWrapper.cpp
  llvm/lib/MC/StringTableBuilder.cpp
  llvm/lib/Support/SmallPtrSet.cpp
  llvm/lib/Target/AMDGPU/AMDGPUGlobalISelUtils.h
  llvm/tools/llvm-c-test/echo.cpp
  mlir/include/mlir/IR/AffineExpr.h
  mlir/include/mlir/IR/DialectInterface.h
  mlir/include/mlir/Support/InterfaceSupport.h
  mlir/include/mlir/Support/StorageUniquer.h

Index: mlir/include/mlir/Support/StorageUniquer.h
===
--- mlir/include/mlir/Support/StorageUniquer.h
+++ mlir/include/mlir/Support/StorageUniquer.h
@@ -12,7 +12,9 @@
 #include "mlir/Support/LLVM.h"
 #include "mlir/Support/LogicalResult.h"
 #include "mlir/Support/TypeID.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Allocator.h"
 
 namespace mlir {
Index: mlir/include/mlir/Support/InterfaceSupport.h
===
--- mlir/include/mlir/Support/InterfaceSupport.h
+++ mlir/include/mlir/Support/InterfaceSupport.h
@@ -14,6 +14,7 @@
 #define MLIR_SUPPORT_INTERFACESUPPORT_H
 
 #include "mlir/Support/TypeID.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/Support/TypeName.h"
 
Index: mlir/include/mlir/IR/DialectInterface.h
===
--- mlir/include/mlir/IR/DialectInterface.h
+++ mlir/include/mlir/IR/DialectInterface.h
@@ -11,6 +11,7 @@
 
 #include "mlir/Support/TypeID.h"
 #include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/STLExtras.h"
 
 namespace mlir {
 class Dialect;
Index: mlir/include/mlir/IR/AffineExpr.h
===
--- mlir/include/mlir/IR/AffineExpr.h
+++ mlir/include/mlir/IR/AffineExpr.h
@@ -17,6 +17,7 @@
 #include "mlir/Support/LLVM.h"
 #include "llvm/ADT/DenseMapInfo.h"
 #include "llvm/Support/Casting.h"
+#include 
 #include 
 
 namespace mlir {
Index: llvm/tools/llvm-c-test/echo.cpp
===
--- llvm/tools/llvm-c-test/echo.cpp
+++ llvm/tools/llvm-c-test/echo.cpp
@@ -18,6 +18,7 @@
 #include "llvm-c/DebugInfo.h"
 #include "llvm-c/Target.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/ErrorHandling.h"
 
 #include 
Index: llvm/lib/Target/AMDGPU/AMDGPUGlobalISelUtils.h
===
--- llvm/lib/Target/AMDGPU/AMDGPUGlobalISelUtils.h
+++ llvm/lib/Target/AMDGPU/AMDGPUGlobalISelUtils.h
@@ -9,6 +9,7 @@
 #ifndef LLVM_LIB_TARGET_AMDGPU_AMDGPUGLOBALISELUTILS_H
 #define LLVM_LIB_TARGET_AMDGPU_AMDGPUGLOBALISELUTILS_H
 
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/CodeGen/Register.h"
 #include 
 
Index: llvm/lib/Support/SmallPtrSet.cpp
===
--- llvm/lib/Support/SmallPtrSet.cpp
+++ llvm/lib/Support/SmallPtrSet.cpp
@@ -13,8 +13,9 @@
 
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/DenseMapInfo.h"
-#include "llvm/Support/MathExtras.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/MathExtras.h"
+#include "llvm/Support/MemAlloc.h"
 #include 
 #include 
 #include 
Index: llvm/lib/MC/StringTableBuilder.cpp
===
--- llvm/lib/MC/StringTableBuilder.cpp
+++ llvm/lib/MC/StringTableBuilder.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "llvm/MC/StringTableBuilder.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/CachedHashString.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
Index: llvm/lib/CodeGen/MBFIWrapper.cpp
===
--- llvm/lib/CodeGen/MBFIWrapper.cpp
+++ llvm/lib/CodeGen/MBFIWrapper.cpp
@@ -11,8 +11,9 @@
 //
 //===--===//
 
-#include "llvm/CodeGen/MBFIWrapper.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/CodeGen/MachineBlockFrequencyInfo.h"
+#include "llvm/CodeGen/MBFIWrapper.h"
 
 using namespace llvm;
 
Index:

[PATCH] D103491: [ADT] Move DenseMapInfo for ArrayRef/StringRef into respective headers (NFC)

2021-06-02 Thread Chris Lattner via Phabricator via cfe-commits 
lattner added inline comments.



Comment at: llvm/include/llvm/ADT/ArrayRef.h:595-600
+  if (RHS.data() == getEmptyKey().data())
+return LHS.data() == getEmptyKey().data();
+  if (RHS.data() == getTombstoneKey().data())
+return LHS.data() == getTombstoneKey().data();
+  return LHS == RHS;
+}

nikic wrote:
> lattner wrote:
> > craig.topper wrote:
> > > lattner wrote:
> > > > I'm pretty sure this method can just be "return LHS == RHS;"  The 
> > > > tombstone/empty comparisons should work without special cases.
> > > Doesn't operator== on ArrayRef compare the elements of the arrays? So 
> > > wouldn't that dereference the invalid pointers used by tombstone/empty?
> > Yes, implemented in terms of std::equal.  However, both of these cases have 
> > zero elements, so the "pointer" is never dereferenced.
> As the length is zero, wouldn't the empty key, the tombstone key and an empty 
> ArrayRef all be considered equal, as the data pointer is never compared?
Yes, you're right, this won't work.  Good catch, thanks :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103491

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

[PATCH] D103491: [ADT] Move DenseMapInfo for ArrayRef/StringRef into respective headers (NFC)

2021-06-02 Thread Simon Pilgrim via Phabricator via cfe-commits 
RKSimon accepted this revision.
RKSimon added a comment.

Thanks for dealing with this @nikic

For this refactor, keeping the implementation as close to the existing one 
makes more sense; also personally I'd prefer the missing include fixups to be 
committed first as NFCs but its up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103491

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

[PATCH] D103491: [ADT] Move DenseMapInfo for ArrayRef/StringRef into respective headers (NFC)

2021-06-02 Thread Nikita Popov via Phabricator via cfe-commits 
nikic added inline comments.



Comment at: llvm/include/llvm/ADT/ArrayRef.h:595-600
+  if (RHS.data() == getEmptyKey().data())
+return LHS.data() == getEmptyKey().data();
+  if (RHS.data() == getTombstoneKey().data())
+return LHS.data() == getTombstoneKey().data();
+  return LHS == RHS;
+}

lattner wrote:
> craig.topper wrote:
> > lattner wrote:
> > > I'm pretty sure this method can just be "return LHS == RHS;"  The 
> > > tombstone/empty comparisons should work without special cases.
> > Doesn't operator== on ArrayRef compare the elements of the arrays? So 
> > wouldn't that dereference the invalid pointers used by tombstone/empty?
> Yes, implemented in terms of std::equal.  However, both of these cases have 
> zero elements, so the "pointer" is never dereferenced.
As the length is zero, wouldn't the empty key, the tombstone key and an empty 
ArrayRef all be considered equal, as the data pointer is never compared?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103491

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

[PATCH] D103491: [ADT] Move DenseMapInfo for ArrayRef/StringRef into respective headers (NFC)

2021-06-02 Thread Chris Lattner via Phabricator via cfe-commits 
lattner added inline comments.



Comment at: llvm/include/llvm/ADT/ArrayRef.h:595-600
+  if (RHS.data() == getEmptyKey().data())
+return LHS.data() == getEmptyKey().data();
+  if (RHS.data() == getTombstoneKey().data())
+return LHS.data() == getTombstoneKey().data();
+  return LHS == RHS;
+}

craig.topper wrote:
> lattner wrote:
> > I'm pretty sure this method can just be "return LHS == RHS;"  The 
> > tombstone/empty comparisons should work without special cases.
> Doesn't operator== on ArrayRef compare the elements of the arrays? So 
> wouldn't that dereference the invalid pointers used by tombstone/empty?
Yes, implemented in terms of std::equal.  However, both of these cases have 
zero elements, so the "pointer" is never dereferenced.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103491

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

[PATCH] D103491: [ADT] Move DenseMapInfo for ArrayRef/StringRef into respective headers (NFC)

2021-06-02 Thread Craig Topper via Phabricator via cfe-commits 
craig.topper added inline comments.



Comment at: llvm/include/llvm/ADT/ArrayRef.h:595-600
+  if (RHS.data() == getEmptyKey().data())
+return LHS.data() == getEmptyKey().data();
+  if (RHS.data() == getTombstoneKey().data())
+return LHS.data() == getTombstoneKey().data();
+  return LHS == RHS;
+}

lattner wrote:
> I'm pretty sure this method can just be "return LHS == RHS;"  The 
> tombstone/empty comparisons should work without special cases.
Doesn't operator== on ArrayRef compare the elements of the arrays? So wouldn't 
that dereference the invalid pointers used by tombstone/empty?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103491

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

[PATCH] D103491: [ADT] Move DenseMapInfo for ArrayRef/StringRef into respective headers (NFC)

2021-06-02 Thread Chris Lattner via Phabricator via cfe-commits 
lattner accepted this revision.
lattner added a comment.
This revision is now accepted and ready to land.

> This required adding quite a few additional includes, as many files were 
> relying on various things pulled in by ArrayRef.h.

This is a _good_ thing btw!

Thank you for driving this!




Comment at: llvm/include/llvm/ADT/ArrayRef.h:595-600
+  if (RHS.data() == getEmptyKey().data())
+return LHS.data() == getEmptyKey().data();
+  if (RHS.data() == getTombstoneKey().data())
+return LHS.data() == getTombstoneKey().data();
+  return LHS == RHS;
+}

I'm pretty sure this method can just be "return LHS == RHS;"  The 
tombstone/empty comparisons should work without special cases.



Comment at: llvm/lib/CodeGen/MBFIWrapper.cpp:14
 
+#include "llvm/ADT/Optional.h"
 #include "llvm/CodeGen/MBFIWrapper.h"

This clang format warning is right, plz move under the other #include



Comment at: llvm/lib/Support/SmallPtrSet.cpp:15
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/DenseMapInfo.h"
 #include "llvm/Support/MathExtras.h"

These are also right, plz fix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103491

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

[PATCH] D103491: [ADT] Move DenseMapInfo for ArrayRef/StringRef into respective headers (NFC)

2021-06-01 Thread Nikita Popov via Phabricator via cfe-commits 
nikic created this revision.
nikic added reviewers: lattner, RKSimon.
Herald added subscribers: foad, dcaballe, cota, teijeong, dexonsmith, 
rdzhabarov, tatianashp, msifontes, jurahul, Kayjukh, grosul1, Joonsoo, kerbowa, 
liufengdb, aartbik, lucyrfox, mgester, arpith-jacob, csigg, antiagainst, 
shauheen, rriddle, mehdi_amini, hiraditya, nhaehnle, jvesely, arsenm.
Herald added a reviewer: rriddle.
nikic requested review of this revision.
Herald added subscribers: llvm-commits, cfe-commits, stephenneuendorffer, 
nicolasvasilache.
Herald added projects: clang, MLIR, LLVM.

This is a followup to D103422 . The 
`DenseMapInfo` implementations for `ArrayRef` and `StringRef` are moved into 
the `ArrayRef.h` and `StringRef.h` headers, which means that these two headers 
no longer need to be included by `DenseMapInfo.h`.

This required adding quite a few additional includes, as many files were 
relying on various things pulled in by `ArrayRef.h`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D103491

Files:
  clang/include/clang/AST/ComparisonCategories.h
  llvm/include/llvm/ADT/ArrayRef.h
  llvm/include/llvm/ADT/DenseMapInfo.h
  llvm/include/llvm/ADT/StringRef.h
  llvm/include/llvm/IR/PassInstrumentation.h
  llvm/include/llvm/Support/Threading.h
  llvm/lib/CodeGen/AsmPrinter/WinException.h
  llvm/lib/CodeGen/MBFIWrapper.cpp
  llvm/lib/MC/StringTableBuilder.cpp
  llvm/lib/Support/SmallPtrSet.cpp
  llvm/lib/Target/AMDGPU/AMDGPUGlobalISelUtils.h
  llvm/tools/llvm-c-test/echo.cpp
  mlir/include/mlir/IR/AffineExpr.h
  mlir/include/mlir/IR/DialectInterface.h
  mlir/include/mlir/Support/InterfaceSupport.h
  mlir/include/mlir/Support/StorageUniquer.h

Index: mlir/include/mlir/Support/StorageUniquer.h
===
--- mlir/include/mlir/Support/StorageUniquer.h
+++ mlir/include/mlir/Support/StorageUniquer.h
@@ -12,7 +12,9 @@
 #include "mlir/Support/LLVM.h"
 #include "mlir/Support/LogicalResult.h"
 #include "mlir/Support/TypeID.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Allocator.h"
 
 namespace mlir {
Index: mlir/include/mlir/Support/InterfaceSupport.h
===
--- mlir/include/mlir/Support/InterfaceSupport.h
+++ mlir/include/mlir/Support/InterfaceSupport.h
@@ -14,6 +14,7 @@
 #define MLIR_SUPPORT_INTERFACESUPPORT_H
 
 #include "mlir/Support/TypeID.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/Support/TypeName.h"
 
Index: mlir/include/mlir/IR/DialectInterface.h
===
--- mlir/include/mlir/IR/DialectInterface.h
+++ mlir/include/mlir/IR/DialectInterface.h
@@ -11,6 +11,7 @@
 
 #include "mlir/Support/TypeID.h"
 #include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/STLExtras.h"
 
 namespace mlir {
 class Dialect;
Index: mlir/include/mlir/IR/AffineExpr.h
===
--- mlir/include/mlir/IR/AffineExpr.h
+++ mlir/include/mlir/IR/AffineExpr.h
@@ -17,6 +17,7 @@
 #include "mlir/Support/LLVM.h"
 #include "llvm/ADT/DenseMapInfo.h"
 #include "llvm/Support/Casting.h"
+#include 
 #include 
 
 namespace mlir {
Index: llvm/tools/llvm-c-test/echo.cpp
===
--- llvm/tools/llvm-c-test/echo.cpp
+++ llvm/tools/llvm-c-test/echo.cpp
@@ -18,6 +18,7 @@
 #include "llvm-c/DebugInfo.h"
 #include "llvm-c/Target.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/ErrorHandling.h"
 
 #include 
Index: llvm/lib/Target/AMDGPU/AMDGPUGlobalISelUtils.h
===
--- llvm/lib/Target/AMDGPU/AMDGPUGlobalISelUtils.h
+++ llvm/lib/Target/AMDGPU/AMDGPUGlobalISelUtils.h
@@ -9,6 +9,7 @@
 #ifndef LLVM_LIB_TARGET_AMDGPU_AMDGPUGLOBALISELUTILS_H
 #define LLVM_LIB_TARGET_AMDGPU_AMDGPUGLOBALISELUTILS_H
 
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/CodeGen/Register.h"
 #include 
 
Index: llvm/lib/Support/SmallPtrSet.cpp
===
--- llvm/lib/Support/SmallPtrSet.cpp
+++ llvm/lib/Support/SmallPtrSet.cpp
@@ -14,6 +14,7 @@
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/DenseMapInfo.h"
 #include "llvm/Support/MathExtras.h"
+#include "llvm/Support/MemAlloc.h"
 #include "llvm/Support/ErrorHandling.h"
 #include 
 #include 
Index: llvm/lib/MC/StringTableBuilder.cpp
===
--- llvm/lib/MC/StringTableBuilder.cpp
+++ llvm/lib/MC/StringTableBuilder.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "llvm/MC/StringTableBuilder.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/CachedHashString.h"
 #include "llvm/ADT/SmallString.h"
 #include