[PATCH] D105338: [InstCombine] Revert "Temporarily do not drop volatile stores before unreachable"

Fri Jul 9 07:35:50 PDT 2021

thakis added a comment.

Lifting a comment from IRC:

  [10:26:49]  <thakis_> i don't have an opinion on if this should be done at all, but if there's some good reason to do it
  [10:26:58]  <thakis_> then i think it should go like a) land diag b) wait a bit c) land change
  [10:27:12]  <thakis_> as-is all users basically have to fix all instances of the warning before they can upgrade compilers
  [10:27:51]  <thakis_> if the diag lands first, users can instead a) upgrade and disable warning b) fix warning async c) receive actual codegen change at a later date, when they're hopefully done cleaning up
  [10:28:13]  <thakis_> ( https://bugs.chromium.org/p/chromium/issues/detail?id=1227705 )
  [10:28:35]  <thakis_> i'd guess (but i don't know) that we have many many instances of volatile nulls, given that that's what the diag recommended up until yesterday
  [10:28:43]  <thakis_> spread over many repositories
  [10:29:31]  <thakis_> (…and https://reviews.llvm.org/rGf4877c78c0fc98be47b926439bbfe33d5e1d1b6d landed without review?)

Given that this landed with several people raising concerns and it making life difficult for probably all users of LLVM, I think this and the clang change should be reverted for now. If there's agreement to move forward (which isn't clear to me from this review here, but I didn't read the prior discussions you linked to), then the clang diag should go on well before the actual LLVM change. And there should be a convincing explanation for the benefits of requiring updating (likely) a significant chunk of the world's code for this in the commit message of both changes.

In any case, the current rollout strategy doesn't seem workable to me. (Happy to hear from others, though.)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105338/new/

https://reviews.llvm.org/D105338