[PATCH] D81685: BypassSlowDivision: Fix dropping debug info

[Matt Arsenault via Phabricator via llvm-commits]

arsenm marked 2 inline comments as done.
arsenm added a comment.

In D81685#2088957 <https://reviews.llvm.org/D81685#2088957>, @vsk wrote:

> Re: "This constructs a new IRBuilder and reconstructs the original divides rather than moving the original", you're not changing the algorithm here, just the way locations are assigned, right? Not sure I understand what's meant by 'reconstructs the original divides'.


I mean mechanically it produces a new IR instruction and deletes the old instruction, rather than move the old instruction to the new block



================
Comment at: llvm/lib/Transforms/Utils/BypassSlowDivision.cpp:441
     QuotRemPair Result = createDivRemPhiNodes(Fast, Slow, SuccessorBB);
     Value *CmpV = insertOperandRuntimeCheck(DividendShort ? nullptr : Dividend,
                                             DivisorShort ? nullptr : Divisor);
----------------
vsk wrote:
> Is it simpler to just pass in the builder (which has the location set already)?
I did that originally, but it didn't work in all of the cases (which required getting the DebugLoc from FastDivInsertionTask). Since I had to track it separately anyway, it seemed cleaner to always get it from there


================
Comment at: llvm/test/Transforms/CodeGenPrepare/AMDGPU/bypass-slow-div-debug-info.ll:29
+; FIXME: The debugloc for the rem parts end up with the dbg of the
+; division.
+define <2 x i64> @sdivrem64(i64 %a, i64 %b) {
----------------
vsk wrote:
> I think this is good? At least, it seems better than having the source location of the srem (!10) appear in the fast block.
I would hope the rem would get the location of the original rem, but I guess this is better than nothing (especially for architectures where these will end up merging into SDIVREM/UDIVREM later anyway)


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

https://reviews.llvm.org/D81685