[PATCH] D151492: Add fastmath attributes to llvm.call_intrinsic

This revision was automatically updated to reflect the committed changes. Closed by commit rG9346dc6f675e: Add fastmath attributes to llvm.call_intrinsic (authored by electriclilies, committed by Mogball). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org

[PATCH] D151492: Add fastmath attributes to llvm.call_intrinsic

Mogball accepted this revision. Mogball added inline comments. This revision is now accepted and ready to land. Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td:878 }]; - let arguments = (ins StrAttr:$intrin, Variadic:$args); + let arguments = (ins StrAttr:

[PATCH] D151492: Add fastmath attributes to llvm.call_intrinsic

Mogball added inline comments. Comment at: mlir/lib/Target/LLVMIR/Dialect/LLVMIR/LLVMToLLVMIRTranslation.cpp:98 static LogicalResult convertCallLLVMIntrinsicOp(CallIntrinsicOp &op, llvm::IRBuilderBase &builder, LLVM::ModuleTranslation &moduleTranslat

[PATCH] D151492: Add fastmath attributes to llvm.call_intrinsic

Mogball added inline comments. Comment at: mlir/lib/Target/LLVMIR/Dialect/LLVMIR/LLVMToLLVMIRTranslation.cpp:98 static LogicalResult convertCallLLVMIntrinsicOp(CallIntrinsicOp &op, llvm::IRBuilderBase &builder, LLVM::ModuleTranslation &moduleTranslat

[PATCH] D151492: Add fastmath attributes to llvm.call_intrinsic

Mogball added inline comments. Comment at: mlir/lib/Target/LLVMIR/Dialect/LLVMIR/LLVMToLLVMIRTranslation.cpp:98 static LogicalResult convertCallLLVMIntrinsicOp(CallIntrinsicOp &op, llvm::IRBuilderBase &builder, LLVM::ModuleTranslation &moduleTranslat

[PATCH] D151492: Add fastmath attributes to llvm.call_intrinsic

Mogball added inline comments. Comment at: mlir/lib/Target/LLVMIR/Dialect/LLVMIR/LLVMToLLVMIRTranslation.cpp:98 static LogicalResult convertCallLLVMIntrinsicOp(CallIntrinsicOp &op, llvm::IRBuilderBase &builder, LLVM::ModuleTranslation &moduleTranslat

[PATCH] D151492: Add fastmath attributes to llvm.call_intrinsic

Mogball added inline comments. Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td:870 -def LLVM_CallIntrinsicOp : LLVM_Op<"call_intrinsic"> { +def LLVM_CallIntrinsicOp : LLVM_Op<"call_intrinsic", [DeclareOpInterfaceMethods]> { let summary = "Call to an LLVM in

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

This revision was automatically updated to reflect the committed changes. Closed by commit rGb508c5649f5e: [MLIR] Add a utility to sort the operands of commutative ops (authored by srishti-pm, committed by Mogball). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://review

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

Mogball added a comment. I'm referring to the nitty gritty details of the while loop inside the comparator. It looks pretty tight to me right now. If the operands are already sorted, worst-case each operand is compared against only its neighbours. Unfortunately, without extra bookkeeping, BFS w

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

Mogball accepted this revision. Mogball added a comment. This revision is now accepted and ready to land. Got a few small nits but otherwise LGTM. Thanks for all the hard work! This looks really solid now. I haven't thought too hard about the performance of that while loop but it seems good enou

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

Mogball added inline comments. Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:238 +// Stores the mapping between an operand and its BFS traversal information. +DenseMap operandToItsBFSMap; + srishti-pm wrote: > Mogball wrote: > > Why can't y

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

Mogball added inline comments. Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:238 +// Stores the mapping between an operand and its BFS traversal information. +DenseMap operandToItsBFSMap; + Why can't you sort the OperandBFS directly to avoi

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

Mogball added a comment. I'm not sure how to check the no-op case without constructing at least a queue. Even assuming only 2 commutative operands, if they look the same `depth=1`, then the comparison has to iterate. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://re

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

Mogball added inline comments. Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:329-353 + assert(frontPosition >= 0 && frontPosition < bfsOfOperands.size() && + "`frontPosition` should be valid"); + unsigned positionOfOperandToShift; + bool foundOperandToSh

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

Mogball added inline comments. Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:76 + // `CONSTANT_OP` and `opName` remains "". + type = CONSTANT_OP; +} srishti-pm wrote: > Mogball wrote: > > Constant ops could be sorted by name as well.

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

Mogball added inline comments. Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:523 + shiftTheSmallestUnsortedOperandsToTheSmallestUnsortedPositions( + bfsOfOperands, smallestUnsortedPosition); + And then you'll never need to check `isSo

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

Mogball added a comment. I'm glad the `DenseSet`s are gone, but my three-ish biggest gripes are: - The algorithm is conceptually simple, but there is way more code than is necessary to achieve it. - More comments (excluding "doc" comments) than code is generally not a good sign - The implementa

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

Mogball added inline comments. Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:178 +using UniquePtrOperandBFS = std::unique_ptr; +using ArrayRefOperandBFS = ArrayRef; + Please remove these. They don't improve readability. Repository: rG LLVM Gith

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

Mogball added a comment. In principle, I think the algorithm is fine. I'm pretty sure you can rewrite bits of it to get rid of the map/sets. I'm only concerned about handling attributes. (e.g. `cmpi slt` vs `cmpi sgt`) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

Mogball added inline comments. Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:211 +// If `op` is not commutative, do nothing. +if (!op->template hasTrait()) + return failure(); Mogball wrote: > Please move the body `matchAndRewrite` in

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

Mogball added a comment. I haven't thought too hard about the algorithm itself yet. I'm in the camp of "let's move forward if it works". I have mostly trivial comments. Comment at: clang/docs/tools/clang-formatted-files.txt:8451 mlir/lib/Transforms/SymbolPrivatize.cpp +mlir/l

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

Mogball added a comment. I'm open to iterating in tree. Landing this utility first and then try adding it as a canonicalization SGTM. The string could be replaced with an array of tuples (yuck) for now. An enum (constant, block arg, everything else) plus the OperationName. Attributes need to b

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

Mogball added a comment. On the matter of whether this should be a canonicalization, my concern with this is that if an operation has its own preferred ordering of operands that conflicts with the sort, then this will cause canonicalization to loop infinitely. It's not actually the canonicaliz

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

Mogball added a comment. I need to look at the algorithm in more detail, but I'm not a fan of using a string key. Concatenating strings to make compound keys is not very efficient and potentially brittle. Can you assign unique IDs and use an array of IDs instead? Comment at:

[PATCH] D116488: Add a misc-unused-parameters.CommentOutUnusedParameters to clang-tidy

Mogball added inline comments. Comment at: clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp:157 // will clean this up afterwards. -MyDiag << FixItHint::CreateReplacement( -RemovalRange, (Twine(" /*") + Param->getName() + "*/").str()); +if (Options.