[AMDGPU] [CodeGen] Fold negate llvm.amdgcn.class into test mask

[gandhi21299]

Summary

• Implemented the following fold-negate transformation in the very beginning of AMDGPUCodegenPrepare.cpp:

xor (llvm.amdgcn.class x, mask), -1 --> llvm.amdgcn.class(x, ~mask)

• Added regression tests

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp

1393

This should not be a separate pass over the function. This should be a visitXor function

1444

You don't need to create a constant to check the value

1454

Should check hasOneUse

1458–1469

I don't know why you are looking at all of these extensions. The xor should directly consume the call

1473–1475

You can just dyn_cast<IntrinsicInst>

1480

Since you know it's a constant, you can also 0 the irrelevant high bits

1485

You don't need to collect dead instructions, the xor should always be dead

llvm/test/CodeGen/AMDGPU/amdgpu-codegenprepare-foldnegate.ll

19

Need a negative test with a variable mask. Also a negative test for multiple uses. Plus also could use tests for all of the FP types

27

You shouldn't be trying to look through this zext, this IR should have been optimized to xor on i1

37–39

This didn't do anything

[gandhi21299]

llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp

1458–1469

I am considering the case where the result from the class intrinsic is extended/truncated for some reason. Is it possible to get the IntrinsicInst directly from the xor operand?

[gandhi21299]

llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp

1480

Do I use IRBuilder to do this or is there a simpler way?

[gandhi21299]

• changes requested by @arsenm

[gandhi21299]

• check if one of the operands of the xor instruction is a ConstantInt, fixes two of the tests

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp

21

Definitely should not include this here

30

Shouldn't need this

819–827

I believe PatternMatch has a nicer way to check for a not

835

I'd prefer to just use the dyn_cast below and return on that rather than checking isa first

842

You don't need this intermediate APInt (plus this is required to be an i32)

llvm/test/CodeGen/AMDGPU/amdgpu-codegenprepare-foldnegate.ll

55

It would be a better test to have a non-identical use (i.e. just add a store of the value)

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp

819–827

Plus constants are canonicalized to the RHS, so you don't need to check both

828–829

Invert condition and exit early to reduce indentation

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp

845

Arg->getType()

[gandhi21299]

• changes requested by @arsenm

[gandhi21299]

• changes as requested by Matt

[gandhi21299]

llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp

819–827

After removing the else-if condition, this seems clean enough to me. Perhaps, PatternMatch won't be required any more.

[gandhi21299]

• refreshing diff, only 2 tests should be failing instead of 4

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp

817

No else after return

821–822

Using m_Not would still be clearer

821–823

Using dyn_cast and isa is redundant, just use dyn_cast and check the result

841

There are fewer bits in the test mask than this

llvm/test/CodeGen/AMDGPU/amdgpu-codegenprepare-foldnegate.ll

2

You didn't actually generate these checks. The operand tests are mostly missing

[foad]

Please run all of check-llvm-codegen-amdgpu. I tried your patch and it looks like a couple more tests need updating.

llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp

17–39

I don't think you need any of these changes to the #includes.

815

This check is wrong. It's OK for there to be multiple uses of the Xor, since you replace all of them. It is not OK for there to be other uses of the intrinsic call, since you modify it in-place.

[gandhi21299]

• requested changes
• @foad two tests fail after running ninja check-llvm-codegen-amdgpu

[gandhi21299]

llvm/test/CodeGen/AMDGPU/amdgpu-codegenprepare-foldnegate.ll

2

Can you elaborate on the kind of tests we need?

[foad]

llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp

23

Don't need this because you don't use the IRBuilder for anything

25

Don't need this unless you actually do some pattern matching

28

Don't need this, it's already included by default

826

Don't need this, you don't use it for anything

830–832

This is wrong. If the value was 4 you will xor it with 7 giving 3, but you need to flip all the bits that amdgcn_class cares about, i.e. 10 low order bits. You should either xor with a fixed value of 0x3ff, or perhaps move this enum from AMDGPUInstCombineIntrinsic.cpp to a common header (maybe SIDefines.h?) and add an "ALL" value to it:

260:  case Intrinsic::amdgcn_class: {
261-    enum {
262-      S_NAN = 1 << 0,       // Signaling NaN
263-      Q_NAN = 1 << 1,       // Quiet NaN
264-      N_INFINITY = 1 << 2,  // Negative infinity
265-      N_NORMAL = 1 << 3,    // Negative normal
266-      N_SUBNORMAL = 1 << 4, // Negative subnormal
267-      N_ZERO = 1 << 5,      // Negative zero
268-      P_ZERO = 1 << 6,      // Positive zero
269-      P_SUBNORMAL = 1 << 7, // Positive subnormal
270-      P_NORMAL = 1 << 8,    // Positive normal
271-      P_INFINITY = 1 << 9   // Positive infinity
272-    };

[gandhi21299]

llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp

23

Turns out, it is used in other places. This pass does not compile without including this header.

830–832

Flipping only the low 10 bits fails several other tests, thoughts? @arsenm @foad

[foad]

llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp

830–832

You need to look at why they fail. Probably your patch changes (improves) the generated code, so the tests need to be updated to expect the improved code sequence.

[gandhi21299]

• get lower 10 bits of the test mask by xor-ing with 0x3ff
• updated tests accordingly
• refactoring
• 2 tests still fail from the AMDGPU codegen test suite

[gandhi21299]

I am not too sure what is causing the test CodeGen/AMDGPU/amdgpu-codegenprepare-i16-to-i32.ll to fail. There is no amdgcn class intrinsic being used anywhere in this test case so there should not be any transformation happening. @arsenm

[foad]

I am not too sure what is causing the test CodeGen/AMDGPU/amdgpu-codegenprepare-i16-to-i32.ll to fail. There is no amdgcn class intrinsic being used anywhere in this test case so there should not be any transformation happening.

Your visitXor function overrides the handling of 16-bit xors which were previously handled by visitBinaryOperator.

For the cases you can't handle, instead of return false you need something like return visitBinaryOperator(I).

[gandhi21299]

• now passes all tests, thanks @foad for the fix

[foad]

Looks OK to me, but please wait a day in case other reviewers still have comments.

llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp

816

All ConstantInts have IntegerType so you don't need to check that.

[gandhi21299]

• eliminated RHS IntegerTy check since it is already declared as a ConstantInt

[arsenm]

llvm/test/CodeGen/AMDGPU/amdgpu-codegenprepare-foldnegate.ll

47

Should check the operands

61

Should check the operands

[gandhi21299]

• added checks on operands for the class intrinsic

[gandhi21299]

Thanks for the review process, I will merge this patch in

llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp

 //===-- AMDGPUCodeGenPrepare.cpp ------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 /// \file
 /// This pass does misc. AMDGPU optimizations on IR before instruction
 /// selection.
 //
 //===----------------------------------------------------------------------===//
 
 #include "AMDGPU.h"
 #include "AMDGPUTargetMachine.h"
 #include "llvm/Analysis/AssumptionCache.h"
 #include "llvm/Analysis/ConstantFolding.h"
 #include "llvm/Analysis/LegacyDivergenceAnalysis.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/CodeGen/TargetPassConfig.h"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/InstVisitor.h"
 #include "llvm/IR/IntrinsicsAMDGPU.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/KnownBits.h"
 #include "llvm/Transforms/Utils/IntegerDivision.h"
 
 #define DEBUG_TYPE "amdgpu-codegenprepare"
 
 using namespace llvm;
 
 namespace {
 
 static cl::opt<bool> WidenLoads(
   "amdgpu-codegenprepare-widen-constant-loads",
   cl::desc("Widen sub-dword constant address space loads in AMDGPUCodeGenPrepare"),
   cl::ReallyHidden,
   cl::init(false));
 
 static cl::opt<bool> Widen16BitOps(
   "amdgpu-codegenprepare-widen-16-bit-ops",
   cl::desc("Widen uniform 16-bit instructions to 32-bit in AMDGPUCodeGenPrepare"),
   cl::ReallyHidden,
   cl::init(true));
class AMDGPUCodeGenPrepare : public FunctionPass,
   bool canWidenScalarExtLoad(LoadInst &I) const;
 
 public:
   static char ID;
 
   AMDGPUCodeGenPrepare() : FunctionPass(ID) {}
 
   bool visitFDiv(BinaryOperator &I);
+  bool visitXor(BinaryOperator &I);
 
   bool visitInstruction(Instruction &I) { return false; }
   bool visitBinaryOperator(BinaryOperator &I);
   bool visitLoadInst(LoadInst &I);
   bool visitICmpInst(ICmpInst &I);
   bool visitSelectInst(SelectInst &I);
 
   bool visitIntrinsicInst(IntrinsicInst &I);
if (NewFDiv) {
     FDiv.replaceAllUsesWith(NewFDiv);
     NewFDiv->takeName(&FDiv);
     FDiv.eraseFromParent();
   }
 
   return !!NewFDiv;
 }
 
+bool AMDGPUCodeGenPrepare::visitXor(BinaryOperator &I) {
+  // Match the Xor instruction, its type and its operands
+  IntrinsicInst *IntrinsicCall = dyn_cast<IntrinsicInst>(I.getOperand(0));
+  ConstantInt *RHS = dyn_cast<ConstantInt>(I.getOperand(1));
+  if (!RHS || !IntrinsicCall || !RHS->getType()->isIntegerTy() ||
+      RHS->getSExtValue() != -1)
+    return false;
+
+  // Check if the Call is an intrinsic intruction to amdgcn_class intrinsic
+  // has only one use
+  if (IntrinsicCall->getIntrinsicID() != Intrinsic::amdgcn_class ||
+      !IntrinsicCall->hasOneUse())
+    return false;
+
+  // "Not" the second argument of the intrinsic call
+  ConstantInt *Arg = dyn_cast<ConstantInt>(IntrinsicCall->getOperand(1));
+  if (!Arg)
+    return false;
+
+  IntrinsicCall->setOperand(
+      1, ConstantInt::get(Arg->getType(), Arg->getZExtValue() ^ 0x3ff));
+  I.replaceAllUsesWith(IntrinsicCall);
+  I.eraseFromParent();
+  return true;
+}
+
 static bool hasUnsafeFPMath(const Function &F) {
   Attribute Attr = F.getFnAttribute("unsafe-fp-math");
   return Attr.getValueAsBool();
 }
 
 static std::pair<Value*, Value*> getMul64(IRBuilder<> &Builder,
                                           Value *LHS, Value *RHS) {
   Type *I32Ty = Builder.getInt32Ty();
   Type *I64Ty = Builder.getInt64Ty();
 
   Value *LHS_EXT64 = Builder.CreateZExt(LHS, I64Ty);
   Value *RHS_EXT64 = Builder.CreateZExt(RHS, I64Ty);
   Value *MUL64 = Builder.CreateMul(LHS_EXT64, RHS_EXT64);
   Value *Lo = Builder.CreateTrunc(MUL64, I32Ty);
   Value *Hi = Builder.CreateLShr(MUL64, Builder.getInt64(32));
   Hi = Builder.CreateTrunc(Hi, I32Ty);
bool AMDGPUCodeGenPrepare::doInitialization(Module &M) {
   DL = &Mod->getDataLayout();
   return false;
 }
 
 bool AMDGPUCodeGenPrepare::runOnFunction(Function &F) {
   if (skipFunction(F))
     return false;
 
   auto *TPC = getAnalysisIfAvailable<TargetPassConfig>();
   if (!TPC)
     return false;
 
   const AMDGPUTargetMachine &TM = TPC->getTM<AMDGPUTargetMachine>();
   ST = &TM.getSubtarget<GCNSubtarget>(F);
   AC = &getAnalysis<AssumptionCacheTracker>().getAssumptionCache(F);
   DA = &getAnalysis<LegacyDivergenceAnalysis>();
 

 
 INITIALIZE_PASS_BEGIN(AMDGPUCodeGenPrepare, DEBUG_TYPE,
                       "AMDGPU IR optimizations", false, false)
 INITIALIZE_PASS_DEPENDENCY(AssumptionCacheTracker)
 INITIALIZE_PASS_DEPENDENCY(LegacyDivergenceAnalysis)
 INITIALIZE_PASS_END(AMDGPUCodeGenPrepare, DEBUG_TYPE, "AMDGPU IR optimizations",
                     false, false)
 
 char AMDGPUCodeGenPrepare::ID = 0;
 
 FunctionPass *llvm::createAMDGPUCodeGenPreparePass() {
   return new AMDGPUCodeGenPrepare();
 }

llvm/test/CodeGen/AMDGPU/amdgpu-codegenprepare-foldnegate.ll

+; RUN: opt -mtriple=amdgcn-amd-amdhsa -amdgpu-codegenprepare -verify -S %s -o - | FileCheck %s
+
+declare i1 @llvm.amdgcn.class.f32(float, i32) nounwind readnone
+declare i1 @llvm.amdgcn.class.f64(double, i32) nounwind readnone
+
+; Trivial case, xor instruction should be removed and
+; the second argument of the intrinsic call should be
+; bitwise-negated
+; CHECK: @fold_negate_intrinsic_test_mask
+; CHECK: %1 = call i1 @llvm.amdgcn.class.f32(float %x, i32 1018)
+define i1 @fold_negate_intrinsic_test_mask(float %x) nounwind {
+  %1 = call i1 @llvm.amdgcn.class.f32(float %x, i32 5)
+  %2 = xor i1 %1, -1
+  ret i1 %2
+}
+
+; Trivial case, xor instruction should be removed and
+; the second argument of the intrinsic call should be
+; bitwise-negated
+; CHECK: @fold_negate_intrinsic_test_mask_dbl
+; CHECK: %1 = call i1 @llvm.amdgcn.class.f64(double %x, i32 1018)
+define i1 @fold_negate_intrinsic_test_mask_dbl(double %x) nounwind {
+  %1 = call i1 @llvm.amdgcn.class.f64(double %x, i32 5)
+  %2 = xor i1 %1, -1
+  ret i1 %2
+}
+
+; Negative test: should not transform for variable test masks
+; CHECK: @fold_negate_intrinsic_test_mask_neg_var
+; CHECK: %[[X0:.*]] = alloca i32
+; CHECK: %[[X1:.*]] = load i32, i32* %[[X0]]
+; CHECK: call i1 @llvm.amdgcn.class.f32(float %x, i32 %[[X1]])
+; CHECK: xor
+define i1 @fold_negate_intrinsic_test_mask_neg_var(float %x) nounwind {
+  %1 = alloca i32
+  store i32 7, i32* %1
+  %2 = load i32, i32* %1
+  %3 = call i1 @llvm.amdgcn.class.f32(float %x, i32 %2)
+  %4 = xor i1 %3, -1
+  ret i1 %4
+}
+
+; Negative test: should not transform for multiple uses of the 
+;   intrinsic returned value
+; CHECK: @fold_negate_intrinsic_test_mask_neg_multiple_uses
+; CHECK: %[[X1:.*]] = call i1 @llvm.amdgcn.class.f32
+; CHECK: store i1 %[[X1]]
+; CHECK: %[[X2:.*]] = xor i1 %[[X1]]
+define i1 @fold_negate_intrinsic_test_mask_neg_multiple_uses(float %x) nounwind {
+  %y = alloca i1
+  %1 = call i1 @llvm.amdgcn.class.f32(float %x, i32 7)
+  %2 = xor i1 %1, -1
+  store i1 %1, i1* %y
+  %3 = xor i1 %1, -1
+  ret i1 %2
+}
+
+; Negative test: should not transform for a xor with no operand equal to -1
+; CHECK: @fold_negate_intrinsic_test_mask_neg_one
+; CHECK: %[[X0:.*]] = call i1 @llvm.amdgcn.class.f32
+; CHECK: xor i1 %[[X0]], false
+define i1 @fold_negate_intrinsic_test_mask_neg_one(float %x) nounwind {
+  %1 = call i1 @llvm.amdgcn.class.f32(float %x, i32 7)
+  %2 = xor i1 %1, false
+  ret i1 %2
+}