Page MenuHomePhabricator

Revert "[InstCombine] erase instructions leading up to unreachable"
AbandonedPublic

Authored by stephan.yichao.zhao on Sep 9 2020, 10:51 AM.

Details

Summary

This reverts commit b22910daab95be1ebc6ab8a74190e38130b0e6ef.

See the discussion from https://reviews.llvm.org/D87149

  1. it is fine to remove instructions that can reach an unreachable instruction because running unreachable is undefined, and undefined behavior is retroactive.
  1. however, a volatile access can abort a program. In that case, an unreachable instruction after it is not reachable. So undefined behavior does not happen, and the volatile access cannot be removed.

It is possible to extend llvm::isGuaranteedToTransferExecutionToSuccessor to make this work.

Revert the change temporarily.

Diff Detail

Unit TestsFailed

TimeTest
920 mslinux > libomp.lock::omp_init_lock.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/lock/omp_init_lock.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/lock/Output/omp_init_lock.c.tmp -lm -latomic && /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/lock/Output/omp_init_lock.c.tmp

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2020, 10:51 AM
stephan.yichao.zhao requested review of this revision.Sep 9 2020, 10:51 AM
stephan.yichao.zhao edited the summary of this revision. (Show Details)Sep 9 2020, 10:54 AM
nikic requested changes to this revision.Sep 9 2020, 11:17 AM
nikic added a subscriber: nikic.

however, a volatile access can abort a program. In that case, an unreachable instruction after it is not reachable. So undefined behavior does not happen, and the volatile access cannot be removed.

As already explained in the referenced revision, this is not correct. Volatile accesses are not permitted to trap. LangRef is unambiguous on this point:

The compiler may assume execution will continue after a volatile operation, so operations which modify memory or may have undefined behavior can be hoisted past a volatile operation.

This revision now requires changes to proceed.Sep 9 2020, 11:17 AM

however, a volatile access can abort a program. In that case, an unreachable instruction after it is not reachable. So undefined behavior does not happen, and the volatile access cannot be removed.

As already explained in the referenced revision, this is not correct. Volatile accesses are not permitted to trap. LangRef is unambiguous on this point:

The compiler may assume execution will continue after a volatile operation, so operations which modify memory or may have undefined behavior can be hoisted past a volatile operation.

I'll follow up on this point in the original review thread for continuity.

This revision now requires changes to proceed.Sep 9 2020, 12:08 PM
nikic added a comment.Sep 9 2020, 12:43 PM

Just to be clear, it is okay to revert this, but you need to adjust your rationale in the description to be one of two things:

  • This broke BigProject due to reliance on undefined behavior, we need a day to fix it. Will reapply once done.
  • I plan to (expeditiously) submit an RFC to llvm-dev that changes the behavior of volatile operations in LLVM, undoing the decision from D53184. Will reapply once that discussion is finished.
spatel added a comment.Sep 9 2020, 2:59 PM

Admittedly, zapping stores (volatile or not) was not in the original motivation for the patch, but it seemed nice to see that enhancement.
Reverting until things are sorted out is ok with me, but I agree with @nikic - since the LangRef is clear today about the behavior and that was hashed out on llvm-dev, somebody (not me) needs to rehash that on llvm-dev. Also, you'll need to reopen https://llvm.org/PR47416 and/or propose an alternate fix.

Abandoned this one after @nikic applied https://github.com/llvm/llvm-project/commit/4e413e16216d0c94ada2171f3c59e0a85f4fa4b6
Thank you for everyone's help.

I echo what @nikic said in https://reviews.llvm.org/D87399#2264195
This is a *temporary* stop-gap measure, it will be reverted *soon* unless someone who cares actively comes forward soon.

Alright, i think 3+ months was plenty time,
but as far as i can tell, no one stepped up forwards to deal with it.

@stephan.yichao.zhao unless someone would like to do so now,
i'm going to escalate this: i'll be reverting the temporary patch soon.