Page MenuHomePhabricator

[InstCombine] inttoptr(load) -> load
AbandonedPublic

Authored by lebedev.ri on Oct 5 2020, 11:25 AM.

Diff Detail

Unit TestsFailed

TimeTest
1,230 mslinux > libarcher.parallel::parallel-nosuppression.c
Script: -- : 'RUN: at line 15'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/parallel/parallel-nosuppression.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-nosuppression.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0' /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-nosuppression.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-nosuppression.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/parallel/parallel-nosuppression.c

Event Timeline

lebedev.ri created this revision.Oct 5 2020, 11:25 AM
lebedev.ri requested review of this revision.Oct 5 2020, 11:25 AM
lebedev.ri added inline comments.
llvm/test/Transforms/InstCombine/select.ll
1145–1146

Missing fold

I guess the idea makes sense.

Not sure what the isUnordered() check is doing. And this needs to be address-space aware.

llvm/utils/compare-stats.py
3

Did you mean to include this?

lebedev.ri added a comment.EditedOct 5 2020, 11:59 AM

I guess the idea makes sense.

Aha, finally we're getting somewhere!

Not sure what the isUnordered() check is doing.

This is the restriction on combineLoadToOperationType().
Can we do this unconditionally?

And this needs to be address-space aware.

combineLoadToNewType() already takes care of annotating the new load type with proper original address space, so we should be good.

llvm/utils/compare-stats.py
3

Err, no

Not sure what the isUnordered() check is doing.

This is the restriction on combineLoadToOperationType().
Can we do this unconditionally?

I think we should not do this for atomic operations if the bitwidth is potentially different. That said, I don't see an immediate reason for the type to make an impact otherwise.

Not sure what the isUnordered() check is doing.

This is the restriction on combineLoadToOperationType().
Can we do this unconditionally?

I think we should not do this for atomic operations if the bitwidth is potentially different. That said, I don't see an immediate reason for the type to make an impact otherwise.

The bitwidth is not different, because we've just fixed bitwidth mismatch in the fold above.
Therefore i'll make this unconditional.

Don't restrict to simple/unordered loads.

I don't like the direction of this patch because it will remove inttoptr instructions that were present in the original program. In the same way that optimizations shouldn't introduce inttoptr, they shouldn't fuse them with loads either.
If the original program had the inttoptr cast and you fold it with a load instruction, then you are asking the load to do the cast. This sort of type punning is not ok. I strongly disagree with Chandler that LLVM's memory is not typed. It needs to distinguish between integers and pointers, otherwise I don't know how to make LLVM correct without a significant perf penalty.
If LLVM's memory was untyped, we would have to assume that every pointer load could be doing an implicit inttoptr cast, which would havoc most optimizations. The fact that some optimizations, like sroa, assume that the memory is untyped and most of the others don't is a source of miscompilations.

lebedev.ri abandoned this revision.Oct 5 2020, 2:03 PM

Thanks @nlopes!
This really should not be a comment, but in some actual docs, like langref.
While there, could you please also comment on D88788 and D88806?