[PATCH] D70597: [PHIEliminate] skip dbg instruction when LowerPHINode

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added subscribers: jmorse, aprantl.
bjope added inline comments.


================
Comment at: llvm/test/CodeGen/X86/phi-node-elimination-dbg-invariant.mir:37
+  ; CHECK: bb.2:
+  ; CHECK: DBG_VALUE %1
+  ; CHECK: EH_LABEL 0
----------------
So for the majority of cases with debug info activated (assuming that EH_LABEL is more rare than DBG_VALUE) we get a regression here, since we now start to introduce use-before-def. That is something that we've been trying to get rid of (and hopefully we can add checks in the IR verifier to detect use-before-def for DBG_VALUE instructions in the future).
I don't like that regression.

I think it could be interesting to investigate why lowering of a PHI node results in inserting the COPY:s after labels in the first place. 
Could also investigate it it would it be ok to reorder a sequence of labels and DBG_VALUE by moving the labels to the beginning of such a sequence (or if earlier passes shouldn't be allowed to insert a DBG_VALUE between PHI and labels that are supposed to be "before any COPY instructions that PHI lowering could result in".

As the patch looks right now, someone would need to file a PR after landing this about the "regressions" introduced by this patch.

If the debug-invariance is considered a much more serious problem than the use-before-def for DBG_VALUE, then I suggest to at least avoid skipping past debug instructions when there are no labels. E.g. first move forward "SkipPHIsLabelsAndDebug", and then move backward again as long as the instruction is a debug instruction. That would need to be accompanied by some code comments describing what is going on (and probably also a PR since the solution wouldn't be perfect).

Well, another option is ofcourse to try to selectively handle any use-before-def scenario that appears. Trying to move the DBG_VALUE if that is legal, or invalidate it, or rewrite it to use the source register used in the COPY, etc. But that is kind of tricky. Such a solution would involve moving the DBG_VALUE past the label. If it is ok to move DBG_VALUE past the labels then I think that we are back to my earlier idea about hoisting labels before debug instructions when a sequence of labels and debug instructions are found after a PHI.

Maybe @jmorse or @aprantl has other ideas on how to deal with this? Are my comments making sense?




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

https://reviews.llvm.org/D70597