Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[PET-to-MLIR] handle PET_EXPR_ACCESS without IDs #5

Closed
wants to merge 1 commit into from
Closed

[PET-to-MLIR] handle PET_EXPR_ACCESS without IDs #5

wants to merge 1 commit into from

Conversation

Komisarczyk
Copy link

No description provided.

chelini
chelini reviewed Apr 24, 2020
View reviewed changes
mlir/tools/mlir-pet/Lib/mlirCodegen.cc Outdated

Value MLIRCodegen::createLoad(__isl_take pet_expr *expr) {
LLVM_DEBUG(dbgs() << __func__ << "\n");
assert((pet_expr_get_type(expr) == pet_expr_access) &&
"expect pet_expr_access type");

auto location = builder_.getUnknownLoc();
//is const -> no id? //FIXME: handle types other than int
auto id = pet_expr_access_get_id(expr);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use: auto id = isl::manage(pet_expr_access_get_id(expr)). So you don't need to free at line 158.

mlir/tools/mlir-pet/Lib/mlirCodegen.cc Outdated
auto id = pet_expr_access_get_id(expr);
if (!id) {
int cst;
auto muaff = (isl::manage(pet_expr_access_get_index(expr)));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the outer "("?

mlir/tools/mlir-pet/Lib/mlirCodegen.cc Outdated
auto maps = isl::map::from_union_map(umap);
isl::pw_aff pwaff = muaff.get_pw_aff(0);
pwaff.foreach_piece([&](isl::set s, isl::aff a) -> isl_stat {
auto val = isl_aff_get_constant_val(a.get());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here. Use isl::manage or isl::manage_copy.

mlir/tools/mlir-pet/Lib/mlirCodegen.cc Outdated
pwaff.foreach_piece([&](isl::set s, isl::aff a) -> isl_stat {
auto val = isl_aff_get_constant_val(a.get());
auto str = isl_val_to_str(val);
cst = std::stoi(str);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if it is not int?

mlir/tools/mlir-pet/Test/Inputs/noid.c Outdated
@@ -0,0 +1,6 @@
int main(){
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this test exercise your patch?

mlir/tools/mlir-pet/Lib/mlirCodegen.cc Outdated
@@ -129,26 +129,33 @@ MLIRCodegen::getSymbolInductionVar(__isl_keep pet_expr *expr,
// TODO: check that expr is freed for all the possible
// exit points. Do we want also to free in case we return
// nullptr? Can we do a C++ wrapper around pet_expr?
// FIXME: pet allows also expression that are like:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This FIXME happens only if you reschedule. Please put it back.

@Komisarczyk Komisarczyk marked this pull request as draft April 24, 2020 07:51
chelini pushed a commit that referenced this pull request May 20, 2020
@delcypher
[asan_symbolize] Fix bug handling C++ symbols when using Atos.
5811f3a 
Summary:
The previous code tries to strip out parentheses and anything in between
them. I'm guessing the idea here was to try to drop any listed arguments
for the function being symbolized. Unfortunately this approach is broken
in several ways.

* Templated functions may contain parentheses. The existing approach
messes up these names.
* In C++ argument types are part of a function's signature for the
purposes of overloading so removing them could be confusing.

Fix this simply by not trying to adjust the function name that comes
from `atos`.

A test case is included.

Without the change the test case produced output like:

```
WRITE of size 4 at 0x6060000001a0 thread T0
    #0 0x10b96614d in IntWrapper<void >::operator=> const&) asan-symbolize-templated-cxx.cpp:10
    #1 0x10b960b0e in void writeToA<IntWrapper<void > >>) asan-symbolize-templated-cxx.cpp:30
    #2 0x10b96bf27 in decltype>)>> >)) std::__1::__invoke<void >), IntWrapper<void > >>), IntWrapper<void >&&) type_traits:4425
    #3 0x10b96bdc1 in void std::__1::__invoke_void_return_wrapper<void>::__call<void >), IntWrapper<void > >>), IntWrapper<void >&&) __functional_base:348
    #4 0x10b96bd71 in std::__1::__function::__alloc_func<void >), std::__1::allocator<void >)>, void >)>::operator>&&) functional:1533
    #5 0x10b9684e2 in std::__1::__function::__func<void >), std::__1::allocator<void >)>, void >)>::operator>&&) functional:1707
    #6 0x10b96cd7b in std::__1::__function::__value_func<void >)>::operator>&&) const functional:1860
    #7 0x10b96cc17 in std::__1::function<void >)>::operator>) const functional:2419
    #8 0x10b960ca6 in Foo<void >), IntWrapper<void > >::doCall>) asan-symbolize-templated-cxx.cpp:44
    #9 0x10b96088b in main asan-symbolize-templated-cxx.cpp:54
    #10 0x7fff6ffdfcc8 in start (in libdyld.dylib) + 0
```

Note how the symbol names for the frames are messed up (e.g. #8, #1).

With the patch the output looks like:

```
WRITE of size 4 at 0x6060000001a0 thread T0
    #0 0x10005214d in IntWrapper<void (int)>::operator=(IntWrapper<void (int)> const&) asan-symbolize-templated-cxx.cpp:10
    #1 0x10004cb0e in void writeToA<IntWrapper<void (int)> >(IntWrapper<void (int)>) asan-symbolize-templated-cxx.cpp:30
    #2 0x100057f27 in decltype(std::__1::forward<void (*&)(IntWrapper<void (int)>)>(fp)(std::__1::forward<IntWrapper<void (int)> >(fp0))) std::__1::__invoke<void (*&)(IntWrapper<void (int)>), IntWrapper<void (int)> >(void (*&)(IntWrapper<void (int)>), IntWrapper<void (int)>&&) type_traits:4425
    #3 0x100057dc1 in void std::__1::__invoke_void_return_wrapper<void>::__call<void (*&)(IntWrapper<void (int)>), IntWrapper<void (int)> >(void (*&)(IntWrapper<void (int)>), IntWrapper<void (int)>&&) __functional_base:348
    #4 0x100057d71 in std::__1::__function::__alloc_func<void (*)(IntWrapper<void (int)>), std::__1::allocator<void (*)(IntWrapper<void (int)>)>, void (IntWrapper<void (int)>)>::operator()(IntWrapper<void (int)>&&) functional:1533
    #5 0x1000544e2 in std::__1::__function::__func<void (*)(IntWrapper<void (int)>), std::__1::allocator<void (*)(IntWrapper<void (int)>)>, void (IntWrapper<void (int)>)>::operator()(IntWrapper<void (int)>&&) functional:1707
    #6 0x100058d7b in std::__1::__function::__value_func<void (IntWrapper<void (int)>)>::operator()(IntWrapper<void (int)>&&) const functional:1860
    #7 0x100058c17 in std::__1::function<void (IntWrapper<void (int)>)>::operator()(IntWrapper<void (int)>) const functional:2419
    #8 0x10004cca6 in Foo<void (IntWrapper<void (int)>), IntWrapper<void (int)> >::doCall(IntWrapper<void (int)>) asan-symbolize-templated-cxx.cpp:44
    #9 0x10004c88b in main asan-symbolize-templated-cxx.cpp:54
    #10 0x7fff6ffdfcc8 in start (in libdyld.dylib) + 0
```

rdar://problem/58887175

Reviewers: kubamracek, yln

Subscribers: #sanitizers, llvm-commits

Tags: #sanitizers

Differential Revision: https://reviews.llvm.org/D79597
chelini pushed a commit that referenced this pull request Jun 14, 2020
@hokein
[AST] Fix a clang crash on an invalid for-range statement.
936ec89 
Summary:
crash stack:

```
llvm-project/clang/lib/AST/ASTContext.cpp:2248: clang::TypeInfo clang::ASTContext::getTypeInfoImpl(const clang::Type *) const: Assertion `!A->getDeducedType().isNull() && "cannot request the size of an undeduced or dependent auto type"' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
 #0 0x00000000025bb0bf llvm::sys::PrintStackTrace(llvm::raw_ostream&) llvm-project/llvm/lib/Support/Unix/Signals.inc:564:13
 #1 0x00000000025b92b0 llvm::sys::RunSignalHandlers() llvm-project/llvm/lib/Support/Signals.cpp:69:18
 #2 0x00000000025bb535 SignalHandler(int) llvm-project/llvm/lib/Support/Unix/Signals.inc:396:3
 #3 0x00007f9ef9298110 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x14110)
 #4 0x00007f9ef8d72761 raise /build/glibc-M65Gwz/glibc-2.30/signal/../sysdeps/unix/sysv/linux/raise.c:51:1
 #5 0x00007f9ef8d5c55b abort /build/glibc-M65Gwz/glibc-2.30/stdlib/abort.c:81:7
 #6 0x00007f9ef8d5c42f get_sysdep_segment_value /build/glibc-M65Gwz/glibc-2.30/intl/loadmsgcat.c:509:8
 #7 0x00007f9ef8d5c42f _nl_load_domain /build/glibc-M65Gwz/glibc-2.30/intl/loadmsgcat.c:970:34
 #8 0x00007f9ef8d6b092 (/lib/x86_64-linux-gnu/libc.so.6+0x34092)
 #9 0x000000000458abe0 clang::ASTContext::getTypeInfoImpl(clang::Type const*) const llvm-project/clang/lib/AST/ASTContext.cpp:0:5
```

Reviewers: sammccall

Subscribers: cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D81384
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chelini chelini chelini left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@Komisarczyk @chelini