This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Retain metadata for single loc fusedloc
ClosedPublic

Authored by jpienaar on Dec 12 2021, 2:14 PM.

Details

Summary

If a fusedloc is created with a single location then no fusedloc
was previously created and single location returned instead. In the case
where there is a metadata associated with the location this results in
discarding the metadata. Instead only canonicalize where there is no
loss of information.

Diff Detail

Event Timeline

jpienaar created this revision.Dec 12 2021, 2:14 PM
jpienaar requested review of this revision.Dec 12 2021, 2:14 PM
rriddle accepted this revision.Dec 13 2021, 9:48 AM
rriddle added inline comments.
mlir/lib/IR/Location.cpp
108–109

Might want to add a comment here about not dropping metadata.

112–113

Can you not just pass it directly?

This revision is now accepted and ready to land.Dec 13 2021, 9:48 AM
jpienaar marked an inline comment as done.Dec 15 2021, 8:34 PM
jpienaar added inline comments.
mlir/lib/IR/Location.cpp
112–113

No, funnily that led to a heap use after free asan failure (I was surprised by that too, didn't dig though)

mehdi_amini added inline comments.Dec 15 2021, 8:40 PM
mlir/lib/IR/Location.cpp
112–113

That seems fishy, can you shared the ASAN failure?

Chia-hungDuan added inline comments.Dec 15 2021, 9:51 PM
mlir/lib/IR/Location.cpp
112–113

Also curious about this case, I applied this patch and passed UnknownLoc::get() directly. Didn't see the problem while running the tests under llvm/llvm-project/mlir/test/IR

Saw other failures when run with asan but it doesn't matter to passed UnknownLoc::get() directly or not.

jpienaar marked an inline comment as done.Dec 17 2021, 12:55 PM
jpienaar added inline comments.
mlir/lib/IR/Location.cpp
112–113

Sure,

==60765==ERROR: AddressSanitizer: stack-use-after-scope on address 0xA at pc 0xA bp 0xA sp 0xA
READ of size 8 at 0xA thread T0
    #0 0xA in get_hashable_data<mlir::Location> //llvm-project/llvm/include/llvm/ADT/Hashing.h:379:21
    #1 0xA in llvm::hash_code llvm::hashing::detail::hash_combine_range_impl<mlir::Location const*>(mlir::Location const*, mlir::Location 
const*) //llvm-project/llvm/include/llvm/ADT/Hashing.h:412:45
    #2 0xA in hash_combine_range<const mlir::Location *> //llvm-project/llvm/include/llvm/ADT/Hashing.h:484:10
    #3 0xA in hash_value<mlir::Location> //llvm-project/llvm/include/llvm/ADT/ArrayRef.h:569:12
    #4 0xA in get_hashable_data<llvm::ArrayRef<mlir::Location> > //llvm-project/llvm/include/llvm/ADT/Hashing.h:379:10
    #5 0xA in combine<llvm::ArrayRef<mlir::Location>, mlir::Attribute> //llvm-project/llvm/include/llvm/ADT/Hashing.h:560:63
    #6 0xA in hash_combine<llvm::ArrayRef<mlir::Location>, mlir::Attribute> //llvm-project/llvm/include/llvm/ADT/Hashing.h:608:17
    #7 0xA in hashKey build_asan/tools/mlir/include/mlir/IR/BuiltinLocationAttributes.cpp.inc:123:12
    #8 0xA in getHash<mlir::detail::FusedLocAttrStorage, std::tuple<llvm::ArrayRef<mlir::Location>, mlir::Attribute> > //llvm/mlir/include/mlir/Support/StorageUniquer.h:317:12
    #9 0xA in mlir::detail::FusedLocAttrStorage* mlir::StorageUniquer::get<mlir::detail::FusedLocAttrStorage, mlir::UnknownLoc&, mlir::Attribute&>(llvm::function_ref<void (mlir::detail::FusedLocAttrStorage*)>, mlir::TypeID, mlir::UnknownLoc&, mlir::Attribute&) //llvm/mlir/include/mlir/Support/StorageUniquer.h:194:26
    #10 0xA in get<mlir::FusedLoc, mlir::UnknownLoc &, mlir::Attribute &> //llvm-project/mlir/include/mlir/IR/AttributeSupport.h:192:39
    #11 0xA in get<mlir::UnknownLoc, mlir::Attribute> //llvm-project/mlir/include/mlir/IR/StorageUniquerSupport.h:141:12
    #12 0xA in mlir::FusedLoc::get(llvm::ArrayRef<mlir::Location>, mlir::Attribute, mlir::MLIRContext*) //llvm/mlir/lib/IR/Location.cpp:114:12
    #13 0xA in mlirLocationFusedGet //llvm-project/mlir/lib/CAPI/IR/IR.cpp:141:15
    #14 0xA  (build_asan/tools/mlir/python_packages/mlir_core/mlir/_mlir_libs/_mlir.cpython-39-x86_64-linux-gnu.so+0x29a6a9)

this is with

+ if (locs.empty()) {
+ if (!metadata)
+ return UnknownLoc::get(context);
+ // std::array<Location, 1> loc({UnknownLoc::get(context)});
+ return Base::get(context, UnknownLoc::get(context), metadata);
+ }
+ if (locs.size() == 1 && !metadata)

jpienaar updated this revision to Diff 395210.Dec 17 2021, 1:13 PM
jpienaar marked an inline comment as done.

Update comment & add TODO

mehdi_amini added inline comments.Dec 17 2021, 1:55 PM
mlir/lib/IR/Location.cpp
112–113

I'm worried this points at a bug in:

#9 0xA in mlir::detail::FusedLocAttrStorage* mlir::StorageUniquer::get<mlir::detail::FusedLocAttrStorage, mlir::UnknownLoc&, mlir::Attribute&>(llvm::function_ref<void (mlir::detail::FusedLocAttrStorage*)>, mlir::TypeID, mlir::UnknownLoc&, mlir::Attribute&) //llvm/mlir/include/mlir/Support/StorageUniquer.h:194:26

Where:

// Construct a value of the derived key type.
auto derivedKey = getKey<Storage>(std::forward<Args>(args)...);

Has to return std::tuple<llvm::ArrayRef<mlir::Location>, mlir::Attribute>> but the arguments are mlir::UnknownLoc&, mlir::Attribute& and it'll internally create an ArrayRef from a local copy of the mlir::UnknownLoc&.

River is best to pin point where this happens exactly I think.

This revision was landed with ongoing or failed builds.Jan 4 2022, 3:39 PM
This revision was automatically updated to reflect the committed changes.