- User Since
- Apr 30 2013, 5:34 PM (394 w, 5 d)
NoSideEffect does not seem to fit to me: there is an effect for allocation. It should also write to its output.
Sat, Nov 21
Fri, Nov 20
Are the changes in mlir/lib/Conversion/AsyncToLLVM/AsyncToLLVM.cpp bug fixes? Can you add IR tests for this (and commit separately so there is a proper commit description)?
Thu, Nov 19
Can you expand a bit on the use-case for deleteUserData? When is the user not in control of the lifetime? This seems all kind of tied to the context in the end?
Wed, Nov 18
(Drive by comments)
Oh I didn't read about https://reviews.llvm.org/D87326 before, isn't the work there making obsolete all of the assertions you (and me before) just added by making this behavior safe?
Can you make more clear in the description the motivation for this patch? Is this mostly about iterator invalidation? Is this expected to make code safer?
I'm not sure about making the mental model a bit harder by having different iterator invalidation behavior implicitly based on T, WDYT?
Mon, Nov 16
[mlir] Require std.alloc() ops to have canonical layout during LLVM lowering.
I tweaked slightly the doc and the commit message:
We like to tag commit messages with "NFC" when they don't modify an existing behavior and don't include tests (this change is a bit borderline, it *could* be plumbed and tested with a pass)
Sat, Nov 14
Seems like a "hack" to me, why not fix the source?
Fri, Nov 13
What do you mean by that?
Anyway, for now, the need for ensuring folds result in the same Type is a reality and that is why ElementwiseMappable is defined that way.
(Feel free to just push directly this kind of fixes)
I think it is very unusual to have a library in LLVM that isn't used in LLVM and so does not come with any testing, so yes it'd be better to do so.
I'm not actually sure if llvm-config just relies implicitly on this?
Won't we end up with a different behavior (with respect to iterator invalidation) based on the platform?
It seems like it is following what was done for OpenMP, however OpenMP is exposing LLVM IR Builder which connects it directly to LLVM. The directive part of the OpenMP library isn't tested either (uhhhh) in LLVM, it just does not expose the issue because it is bundles in the same library as the IR builders which are tested.
@clementval : it isn't clear why this is in llvm/lib/Frontend/? Should this be a top-level frontend/lib kind of structure? There isn't any user of this inside LLVM right now other than flang and it isn't clear to me how it connects to LLVM itself.
I send out https://reviews.llvm.org/D91470 as another take on this.
I know this fixes a bug, but I don't think this is quite correct.
Fix one clang instance failing this assertion
Update with Sean's suggestion
It fails for me as well: configuring just LLVM alone and running ninja check-llvm instead of building first everything with ninja alone.
+1 on both points!
Thu, Nov 12
SameOperandsAndResultShape allows `"foo"(%0) : tensor<2xf32> -> tensor<?xf32> but ElementwiseMappable does not.
You may need a refactor like Alex did here: https://reviews.llvm.org/D85650 ; what about a factory method returning ErrorOr<Triple> ?
Can you add a test for this?
Wed, Nov 11
Do you have tests for all this?
I'd prefer to avoid SmallVec: it is too close to SmallVector (it is even a prefix, which will conflict with autocompletion in IDEs).
llvm::SVec or llvm::SVector on the other hand seems different enough to me to avoid confusion, and the using definition you propose seems like a good tradeoff to me!
DefaultSmallVector is quite a "mouthful" though, I'd have thought we'd look for a "default container" that is easy to type if we have it everywhere.
Reverted in f917356f9ce0 ; I suspect a static constexpr in a class missing a definition out of class (required pre-c++17).