[PATCH] D123473: [LICM] Only create load in pre-header when promoting load.

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Coming into this quite late, but I'm concerned about this patch and the reasoning discussed in this thread.

Consider the fact that the original program may have contained a load along a dynamically dead path inside a loop contained by a writeonly function.  Even if we interpret the load as being UB when executed, the original program is well defined.  If we then speculatively insert a load into the preheader - which we do based on speculation safety, even with this patch - then the load will execute in a writeonly function.

Before:

  loop { 
    v = 0
    if (never_taken) {
      v = load addr
    }
    store v, addr
  }

After:

  v1 = load addr
  loop { 
    v = 0
    if (never_taken) {
      v = v1
    }
    store v, addr
  }

If we consider such a load immediate UB, then this is not a valid transform.  However, consider that this transform is the same as performed by LICM (e.g. not store promotion.)(

If we actually want to prevent this transform, I think we'd have to consider loads in writeonly functions not safe to speculate.

I don't think we actually want to do that.  Instead, I think we're making a mistake by considering the load to be immediate UB as opposed to simply returning poison in this case.

If we accept that semantic, then this patch becomes completely unnecessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123473