[PATCH] D156706: AMDGPU: Simplify and improve sincos matching

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULibCalls.cpp:1164-1168
+  Sin->replaceAllUsesWith(Call);
+  Sin->eraseFromParent();
+
+  Cos->replaceAllUsesWith(Reload);
+  Cos->eraseFromParent();
----------------
arsenm wrote:
> jmmartinez wrote:
> > arsenm wrote:
> > > jmmartinez wrote:
> > > > We should also set the debug location for the `Call` to be the one of `Sin`, and the debug locaiton of `Cos` to be the one of `Reload`
> > > I couldn't figure out what to do about the debug loc. There didn't seem to be a update-these-two-for-merge function anywhere. Currently it will get the debug loc for the initial call
> > There is `Instruction::applyMergedLocation` but it doesn't seem appropiate in this case.
> > 
> > If I'm not wrong, crrently it's getting the debug location of the `IRBuilder` [insert point](https://llvm.org/doxygen/IRBuilder_8h_source.html#l00196) (and only if `Arg` is an instruction).
> You're half wrong. The default is the location for the initially visited instruction. If the incoming argument is an instruction, it breaks it by taking the location from the argument. It's not unset anywhere
I'm also not sure SetInsertPointPastAllocas is doing the right thing by *not* resetting the debug location


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

https://reviews.llvm.org/D156706