[PATCH] D75817: [NVPTX] Fix instruction selection for addresses in case of addrspacecasts

[Thomas Faingnaert via Phabricator via llvm-commits]

thomasfaingnaert added a comment.

In D75817#1912891 <https://reviews.llvm.org/D75817#1912891>, @tra wrote:

> While such reordering may be beneficial, in general, `GEP(ASC(x))` is not always equivalent of `ASC(GEP(x))`, so we can't just blindly do it. I believe this has been discussed few times in the past, though I can't find relevant emails now.


I actually stumbled upon this issue because `opt` reordered the `GEP` and `ASC`, i.e.  the input above gets converted to the following using `opt -O3`:

  ; ModuleID = 'tmp.ll'
  source_filename = "tmp.ll"
  target triple = "nvptx64-nvidia-cuda"
  
  ; Function Attrs: nofree norecurse nounwind writeonly
  define void @bad(i64) local_unnamed_addr #0 {
    %ptr = inttoptr i64 %0 to i16*
    %gep = getelementptr i16, i16* %ptr, i64 16
    %asc = addrspacecast i16* %gep to i16 addrspace(1)*
    store i16 0, i16 addrspace(1)* %asc, align 16
    ret void
  }
  
  ; Function Attrs: nofree norecurse nounwind writeonly
  define void @good(i64) local_unnamed_addr #0 {
    %ptr = inttoptr i64 %0 to i16*
    %gep1 = getelementptr i16, i16* %ptr, i64 16
    %gep = addrspacecast i16* %gep1 to i16 addrspace(1)*
    store i16 0, i16 addrspace(1)* %gep, align 16
    ret void
  }
  
  attributes #0 = { nofree norecurse nounwind writeonly }

As you can see, the `good` function was changed so that the `GEP` precedes the `ASC`.

I assumed this to be because `ASC` after `GEP` is the canonical form, and `opt` reordered these to make the life of the backends easier.
But I suppose that's not the case, and my real issue is that `opt` shouldn't have reordered `GEP` and `ASC` in the first place?

I did some more testing, and it appears that that the `InstCombine` pass (`opt -instcombine`) is responsible for this.
Maybe it would be better to ensure `InstCombine` doesn't reorder instead of fixing this in instruction selection as I do now?
I'm not sure if that would influence other backends that rely on this reordering, though.


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

https://reviews.llvm.org/D75817