The utility function should live in StaticValueUtils.h as it provides
a convenient way to convert a vector of OpFoldResults into a vector of
Values.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 188198 Build 284376: arc lint + arc unit
Event Timeline
mlir/include/mlir/Dialect/Utils/StaticValueUtils.h | ||
---|---|---|
83–84 | Nit: please document that this expects constant values to be IntegerAttributes and that it will create Index-typed constants. |
This change introduces a circular build dependence: DialectUtils <- ArithmeticUtils <- ArithDialect <- DialectUtils.
chelini: can you either revert or break the cycle? I don't see a trivial way of doing the latter.
I think you talk about the Bazel build, right? Actually I think the ArithDialect to DialectUtils dependency is unused. Yesterday when this change landed first, I had the Bazel build working by removing that unused dependency.
Please check https://github.com/llvm/llvm-project/commit/ec8f08ce08c26f6db61b142ba40590c3c1d158a0 and https://github.com/llvm/llvm-project/commit/b4cc363e86e7506e41560af333b12beefd9f4bbd
@chelini if you roll forward, please consider updating the Bazel BUILD files like I did in the two revisions mentioned above
The Bazel build isn't officially supported by the LLVM community and should not be considered a strong motive for reverting patches, nor it should be a requirement that people build with Bazel *before* landing their patches on generic LLVM code.
If the Bazel community wants to have that kind of support in LLVM, then they'll need a new round of RFCs and convince everyone that this is a good idea. IMO, this will not happen any time soon.
@chelini, if this is the last problem with your patch, feel free to re-land it and let the Bazel folks fix this upstream on their end.
cheers,
--renato
Sorry, I didn't intend to say that it is required to fix the Bazel build when relanding the change. My intent was just to say it would be nice if that is done, given that it is just applying the two patches I pointed to. We can certainly also do this ourselves.
@akuegel is right, my apologies. I wrongly assumed that in Cmake, ArithDialect depends on DialectUtils, just like it (unnecessarily) does in Bazel. I confirm that removing that superfluous dependence fixes the cycle in Bazel.
@chelini: please feel free to reland this and I will update the Bazel files.
@rengolin: AFAIK we do not want dependency cycles in the build system (Cmake or otherwise). I wrongly thought I was reporting one such case.
Nit: please document that this expects constant values to be IntegerAttributes and that it will create Index-typed constants.