[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

1414

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

1465

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

1475

Should check hasOneUse

1479–1490

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

1494–1496

You can just dyn_cast<IntrinsicInst>

1501

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

1506

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

1479–1490

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

1501

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

23

Definitely should not include this here

41

Shouldn't need this

831–839

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

847

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

854

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

831–839

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

840–841

Invert condition and exit early to reduce indentation

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp

857

Arg->getType()

[gandhi21299]

• changes requested by @arsenm

[gandhi21299]

• changes as requested by Matt

[gandhi21299]

llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp

831–839

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

829

No else after return

833–834

Using m_Not would still be clearer

833–835

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

853

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.

827

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

28

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

33–34

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

36

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

838

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

842–844

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

28

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

842–844

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

[foad]

llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp

842–844

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

828

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-c/Core.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/Analysis/AssumptionCache.h"
 #include "llvm/Analysis/ConstantFolding.h"
 #include "llvm/Analysis/LegacyDivergenceAnalysis.h"
 #include "llvm/Analysis/ValueTracking.h"
+#include "llvm/CodeGen/GlobalISel/InstructionSelector.h"
 #include "llvm/CodeGen/TargetPassConfig.h"
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/Constants.h"
 #include "llvm/IR/Dominators.h"
+#include "llvm/IR/GlobalValue.h"
+#include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/InstVisitor.h"
+#include "llvm/IR/Instruction.h"
+#include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicsAMDGPU.h"
-#include "llvm/IR/IRBuilder.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Debug.h"
 #include "llvm/Support/KnownBits.h"
+#include "llvm/Transforms/Utils/BasicBlockUtils.h"
 #include "llvm/Transforms/Utils/IntegerDivision.h"
+#include <bits/stdint-uintn.h>
 
 #define DEBUG_TYPE "amdgpu-codegenprepare"
 
 using namespace llvm;
 
 namespace {
 
 static cl::opt<bool> WidenLoads(
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 and its operands
+  CallInst *Call = nullptr;
+  if (I.hasOneUse()) {
+    if (isa<CallInst>(I.getOperand(0)) &&
+        dyn_cast<ConstantInt>(I.getOperand(1))->getSExtValue() == -1)
+      Call = cast<CallInst>(I.getOperand(0));
+    else if (isa<CallInst>(I.getOperand(1)) &&
+             dyn_cast<ConstantInt>(I.getOperand(0))->getSExtValue() == -1)
+      Call = cast<CallInst>(I.getOperand(1));
+    else
+      return false;
+  } else
+    return false;
+
+  // Check if the Call is an intrinsic intruction to amdgcn_class intrinsic
+  IntrinsicInst *IntrinsicCall = dyn_cast<IntrinsicInst>(Call);
+  if (!IntrinsicCall ||
+      IntrinsicCall->getIntrinsicID() != Intrinsic::amdgcn_class ||
+      !isa<Constant>(IntrinsicCall->getOperand(1)))
+    return false;
+
+  // "Not" the second argument of the intrinsic call
+  IRBuilder<> Builder(IntrinsicCall);
+  ConstantInt *Arg = dyn_cast<ConstantInt>(IntrinsicCall->getOperand(1));
+  int64_t BitWidth = Arg->getBitWidth();
+  APInt NewArg(BitWidth, ~Arg->getZExtValue());
+
+  IntrinsicCall->setOperand(
+      1, ConstantInt::get(IntrinsicCall->getOperand(1)->getType(), NewArg));
+  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();
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

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; 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 -6)
+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 -6)
+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: %[[X0:.*]] = call i1 @llvm.amdgcn.class.f32
+; CHECK: %[[X1:.*]] = xor i1 %[[X0]]
+; CHECK: xor i1 %[[X1]]
+; CHECK: xor i1 %[[X1]]
+define i1 @fold_negate_intrinsic_test_mask_neg_multiple_uses(float %x) nounwind {
+  %1 = call i1 @llvm.amdgcn.class.f32(float %x, i32 7)
+  %2 = xor i1 %1, -1
+  %3 = xor i1 %2, -1
+  %4 = xor i1 %2, -1
+  ret i1 %3
+}
+
+; 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
+}