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

[Nuno Lopes via Phabricator via llvm-commits]

nlopes added a comment.

In D123473#3485355 <https://reviews.llvm.org/D123473#3485355>, @nlopes wrote:

> In D123473#3484702 <https://reviews.llvm.org/D123473#3484702>, @reames wrote:
>
>> 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.
>
> You've a point. My concern is that it's surprising to have LLVM introducing a load in a function that's not supposed to do any load. I was thinking about I/O, but those accesses should be volatile. For kernel code it may not be ok, but probably they also use volatile to prevent the compiler messing around instead of relying on `writeonly`

I run LLVM's test suite under Alive2 and found no issues with the load-is-poison semantics for writeonly & argmemonly functions.

Giving it a second thought, I agree with you. These attributes are not there to prevent optimizations, but to enable them. The compiler can do whatever it wants as long as it respects the memory model. The kernel may have a different idea about how the memory model looks like, but for now we don't support such special casing like gcc does (or used to).

So I think we can change it. It's generally better to keep UB usage as low as possible, and yield poison whenever possible. So I favor the change.

As for this patch, it optimizes the case where the loop & store are guaranteed to execute, so no load is needed. This is independent of writeonly.
See this example:

  define void @test(i8 %var, ptr %ptr) {
  entry:
    br label %for.cond
  
  for.cond:
    %i = phi i64 [ 0, %entry ], [ %add, %cond.end ]
    %cmp = icmp ult i64 %i, 2
    br i1 %cmp, label %for.body39, label %for.end
  
  for.body39:
    %div = sdiv i8 %var, 3
    %cmp2 = icmp slt i8 %div, 0
    br i1 %cmp2, label %cond.true, label %cond.false
  
  cond.true:
    br label %cond.end
  
  cond.false:
    br label %cond.end
  
  cond.end:
    %merge = phi i8 [ %div, %cond.true ], [ 0, %cond.false ]
    store i8 %merge, ptr %ptr, align 1
    %add = add i64 %i, 4
    br label %for.cond
  
  for.end:
    ret void
  }

LICM currently produces a load that is replaced with poison with this patch:

  define void @test(i8 %var, ptr %ptr) {
  entry:
    %div = sdiv i8 %var, 3
    %cmp2 = icmp slt i8 %div, 0
    %ptr.promoted = load i8, ptr %ptr, align 1
    br label %for.cond
  ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123473