[PATCH] D147721: [AMDGPU] Fix a case of updating LiveIntervals in SIOptimizeExecMaskingPreRA

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

> I assume D129208 <https://reviews.llvm.org/D129208> is the test case for this, so it does not need any new tests?

Yes that is my thinking. To test it in isolation I would need to tweak the RUN line in `test/CodeGen/AMDGPU/optimize-negated-cond-exec-masking.mir` to run another pass after si-optimize-exec-masking-pre-ra which depends on LiveIntervals having been preserved. I suppose I could try that, but it would become redundant again after D129208 <https://reviews.llvm.org/D129208> lands.



================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMaskingPreRA.cpp:109
 //    %sel = V_CNDMASK_B32_e64 0, 1, %cc
-//    %cmp = V_CMP_NE_U32 1, %1
+//    %cmp = V_CMP_NE_U32 1, %sel
 //    $vcc = S_AND_B64 $exec, %cmp
----------------
critson wrote:
> Presumably this is just fixing an error in the original comment, i.e. NFC?
Right. I've committed it separately now: b2e98c18cc8abf2c1cc6a7f85c8708550cc8fd28


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMaskingPreRA.cpp:303
+        LIS->shrinkToUses(SelLI);
+      }
     }
----------------
critson wrote:
> I take it we cannot simply move the first `LIS->shrinkToUses(SelLI);` after the `if` because the `IsDead` query depends on it?
Yes. On a related note, instead of rolling our own IsDead test I wondered if we should use the form of shrinkToUses which returns a vector of dead instructions. I tried that but it was causing other CodeGen differences not strictly related to this bug fix so I gave up.

Going even further, perhaps we should be using something like LiveRangeEdit::eliminateDeadDefs which already has the logic to iterate calling shrinkToUses.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147721