[Mlir-commits] [mlir] [mlir] Speed up FuncToLLVM: CallOpLowering using a hashtable (PR #68082)

[Oleksandr Alex Zinenko]

================
@@ -601,19 +602,38 @@ struct CallOpInterfaceLowering : public ConvertOpToLLVMPattern<CallOpType> {
   }
 };
 
-struct CallOpLowering : public CallOpInterfaceLowering<func::CallOp> {
-  using Super::Super;
+class CallOpLowering : public CallOpInterfaceLowering<func::CallOp> {
+public:
+  CallOpLowering(
+      const LLVMTypeConverter &typeConverter,
+      // Can be nullptr.
+      const std::unordered_map<std::string, bool> *hasBarePtrAttribute,
+      PatternBenefit benefit = 1)
+      : CallOpInterfaceLowering<func::CallOp>(typeConverter, benefit),
+        barePtrCallConvForcedByOptions(
+            typeConverter.getOptions().useBarePtrCallConv),
+        hasBarePtrAttribute(hasBarePtrAttribute) {}
 
   LogicalResult
   matchAndRewrite(func::CallOp callOp, OpAdaptor adaptor,
                   ConversionPatternRewriter &rewriter) const override {
     bool useBarePtrCallConv = false;
-    if (Operation *callee = SymbolTable::lookupNearestSymbolFrom(
+    if (barePtrCallConvForcedByOptions) {
+      useBarePtrCallConv = true;
+    } else if (hasBarePtrAttribute != nullptr) {
+      useBarePtrCallConv =
+          hasBarePtrAttribute->at(callOp.getCalleeAttr().getValue().str());
+    } else if ( // Warning: This is a linear lookup.
+        Operation *callee = SymbolTable::lookupNearestSymbolFrom(
             callOp, callOp.getCalleeAttr())) {
-      useBarePtrCallConv = shouldUseBarePtrCallConv(callee, getTypeConverter());
+      useBarePtrCallConv = callee->hasAttr(barePtrAttrName);
     }
     return matchAndRewriteImpl(callOp, adaptor, rewriter, useBarePtrCallConv);
   }
+
+private:
+  bool barePtrCallConvForcedByOptions = false;
+  const std::unordered_map<std::string, bool> *hasBarePtrAttribute = nullptr;
----------------
ftynse wrote:

Have you tried `llvm::StringMap`? It was designed for such cases and is less wasteful in storage. There are also more containers https://llvm.org/docs/ProgrammersManual.html#map-like-containers-std-map-densemap-etc.

Even better, I suspect you can map symbol _attributes_ instead of materializing their names as strings. Attributes are pointers to context-owned data, it will be cheaper to compare those than to hash strings. Something like `SmallPtrSet`, or even a vector with linear lookup for locality reasons, may be used to store specifically the pointers that have the attribute and assume there's no attribute if the op is not in the cache.

https://github.com/llvm/llvm-project/pull/68082