[PATCH] D146287: [AMDGPU][GISel] Add inverse ballot intrinsic

[Jessica Del via Phabricator via llvm-commits]

OutOfCache marked 7 inline comments as done.
OutOfCache added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:1380
+
+  for (unsigned i = 0; i < NumParts; ++i) {
+    Register DstPart = MRI->createVirtualRegister(&AMDGPU::SReg_32RegClass);
----------------
foad wrote:
> Emitting the readfirstlanes directly during instruction selection is pretty unusual. Usually it is done by legalizeOperands during SIFixSGPRCopies. Is there a reason that inverse ballot has to be done differently?
Turns out there isn't! Thank you for the suggestion. It is much cleaner now.


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.inverse.ballot.i32.ll:3
+; RUN: llc -march=amdgcn -mcpu=gfx1010 -mattr=+wavefrontsize32,-wavefrontsize64 -global-isel -amdgpu-global-isel-risky-select -verify-machineinstrs < %s | FileCheck -check-prefixes=GFX10 %s
+; RUN: llc -march=amdgcn -mcpu=gfx1100 -amdgpu-enable-delay-alu=0 -mattr=+wavefrontsize32,-wavefrontsize64 -global-isel -amdgpu-global-isel-risky-select -verify-machineinstrs < %s | FileCheck  -check-prefixes=GFX11 %s
+
----------------
rovka wrote:
> I have 2 nits here:
> 1. Why go all the way to assembly? You could -stop-after=finalize-isel (or even before it). You'll probably have to use update_mir_test_checks.py though.
> 2. There's precedent for having a single test with -global-isel=0 and -global-isel=1 RUN lines, so you don't really need separate files in this directory.
Thank you for the suggestion. Combining the GISel and SelDAG Tests into one file is much better. 

I run into an issue when trying the -stop-after=finalize-isel however. The CHECK lines seem to be generated fine with update_mir_test_checks.py, but running the tests with llvm-lit fails the tests. Changing back to asm tests works again, with no other changes to the code. I will need to investigate further.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146287