[PATCH] D18072: Skeleton for the IR level pass to perform 64bit Integer Division
Matt Arsenault via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 10 17:46:07 PST 2016
arsenm added inline comments.
================
Comment at: include/llvm/CodeGen/Passes.h:395-397
@@ -394,2 +394,5 @@
+ /// Lowers unsupported integer division.
+ extern char &IntegerDivisionID;
+
/// MachineLoopInfo - This pass is a loop analysis pass.
----------------
These parts touching generic code should be removed since it is a backend pass now. The IDs etc. declarations should be only in AMDGPU.h
================
Comment at: lib/Target/AMDGPU/AMDGPU.h:89
@@ -84,1 +88,3 @@
+/*Modified Integer Division : IR Pass to perform 64bit integer division*/
+
----------------
Description comments should be in the pass file instead
================
Comment at: lib/Target/AMDGPU/AMDGPU64bitDivision.cpp:8
@@ +7,3 @@
+
+bool llvm::AMDExpandUDivision(BinaryOperator *Div)
+{
----------------
There should not be a free function like this. This should be a method of the pass
================
Comment at: lib/Target/AMDGPU/AMDGPU64bitDivision.h:1-2
@@ +1,3 @@
+#include "AMDGPUISelLowering.h"
+#include "AMDGPU.h"
+#include "AMDGPUDiagnosticInfoUnsupported.h"
----------------
This header is unnecessary, this can be done entirely in the one cpp file
================
Comment at: lib/Target/AMDGPU/AMDGPU64bitDivision.h:3-6
@@ +2,6 @@
+#include "AMDGPU.h"
+#include "AMDGPUDiagnosticInfoUnsupported.h"
+#include "AMDGPUFrameLowering.h"
+#include "AMDGPUIntrinsicInfo.h"
+#include "AMDGPURegisterInfo.h"
+#include "AMDGPUSubtarget.h"
----------------
You don't need these
================
Comment at: lib/Target/AMDGPU/AMDGPU64bitDivision.h:8-14
@@ +7,9 @@
+#include "AMDGPUSubtarget.h"
+#include "R600MachineFunctionInfo.h"
+#include "SIMachineFunctionInfo.h"
+#include "llvm/CodeGen/CallingConvLower.h"
+#include "llvm/CodeGen/MachineFunction.h"
+#include "llvm/CodeGen/MachineRegisterInfo.h"
+#include "llvm/CodeGen/SelectionDAG.h"
+#include "llvm/CodeGen/TargetLoweringObjectFileImpl.h"
+#include "llvm/IR/DataLayout.h"
----------------
Also any of these
================
Comment at: lib/Target/AMDGPU/AMDGPUISelLowering.cpp:256-257
@@ -255,2 +255,4 @@
setOperationAction(ISD::CTLZ, VT, Expand);
+
+ setOperationAction(ISD::CTLZ_ZERO_UNDEF, VT, Expand);
}
----------------
This looks like an unrelated change, and is also incorrect since this should work already. i32 has an instruction and i64 should already be expanded
================
Comment at: lib/Target/AMDGPU/AMDGPUISelLowering.cpp:259-261
@@ -256,3 +258,5 @@
}
+
+
if (!Subtarget->hasBCNT(32))
----------------
Random whitespace change
================
Comment at: lib/Target/AMDGPU/AMDGPUIntegerDivisionPass.cpp:1
@@ +1,2 @@
+//===-- IntegerDivisionPass.cpp - Expand div/mod instructions -------------===//
+//
----------------
Name out of date
================
Comment at: lib/Target/AMDGPU/AMDGPUIntegerDivisionPass.cpp:9-11
@@ +8,5 @@
+//===----------------------------------------------------------------------===//
+//
+/// \file
+//===----------------------------------------------------------------------===//
+
----------------
This should have a description or be removed
================
Comment at: lib/Target/AMDGPU/AMDGPUIntegerDivisionPass.cpp:13
@@ +12,3 @@
+
+/*Modified Integer Division Pass*/
+
----------------
AMDGPU.h should be the first included file
================
Comment at: lib/Target/AMDGPU/AMDGPUIntegerDivisionPass.cpp:27
@@ +26,3 @@
+
+#define DEBUG_TYPE "AMDGPU integer-division"
+
----------------
This should be all lower case and - separated, otherwise it won't really work as a -debug-only= argument
================
Comment at: lib/Target/AMDGPU/AMDGPUIntegerDivisionPass.cpp:37
@@ +36,3 @@
+
+ int checkingVariable;
+
----------------
This should be removed
================
Comment at: lib/Target/AMDGPU/AMDGPUIntegerDivisionPass.cpp:67
@@ +66,3 @@
+char AMDGPUIntegerDivision::ID = 0;
+//char &llvm::IntegerDivisionID = AMDGPUIntegerDivision::ID;
+INITIALIZE_TM_PASS(AMDGPUIntegerDivision, DEBUG_TYPE,"Expand integer division", false, false);
----------------
Dead code
================
Comment at: lib/Target/AMDGPU/AMDGPUIntegerDivisionPass.cpp:70-74
@@ +69,7 @@
+
+char &llvm::AMDGPUIntegerDivisionID = AMDGPUIntegerDivision::ID;
+/*INITIALIZE_PASS_BEGIN(AMDGPUIntegerDivision, DEBUG_TYPE,
+ "Add AMDGPU function attributes", false, false)
+INITIALIZE_PASS_END(AMDGPUIntegerDivision, DEBUG_TYPE,
+ "Add AMDGPU function attributes", false, false)*/
+
----------------
Dead code
================
Comment at: lib/Target/AMDGPU/AMDGPUIntegerDivisionPass.cpp:90-91
@@ +89,4 @@
+ BasicBlock *BB = &*BBI;
+ for (BasicBlock::iterator II = BB->begin(), IE = BB->end(); II != IE; ++II) {
+ Instruction *I = &*II;
+ if (visit(*I)) {
----------------
You're going to need to pre-increment this pointer to avoid iterator invalidation issues when you delete the original div instruction
================
Comment at: lib/Target/AMDGPU/AMDGPUIntegerDivisionPass.cpp:93
@@ +92,3 @@
+ if (visit(*I)) {
+ BBI = F.begin();
+ break;
----------------
You can pre-increment the iterators to avoid skipping back to the beginning
================
Comment at: lib/Target/AMDGPU/AMDGPUIntegerDivisionPass.cpp:106
@@ +105,3 @@
+ // TODO:Uthkarsh later modify to handle signed 64 bit too.
+ bool isUdiv64 = I.getOpcode() == Instruction::UDiv && I.getType()->getIntegerBitWidth() == 64;
+ return shouldExpandInIr && isUdiv64;
----------------
You can do Type->isIntegerTy(64)
================
Comment at: lib/Target/AMDGPU/AMDGPUIntegerDivisionPass.cpp:150
@@ +149,2 @@
+
+//static RegisterPass<AMDGPUIntegerDivision> X("AMD GPU Integer Division", "IR level 64 bit integer division",false,false);
----------------
This shouldn't be here
================
Comment at: lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:39
@@ -38,2 +38,3 @@
+
using namespace llvm;
----------------
Random whitespace
================
Comment at: lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:56
@@ -54,1 +55,3 @@
initializeAMDGPUAnnotateUniformValuesPass(*PR);
+ /*Modified Integer Division Pass*/
+ initializeAMDGPUIntegerDivisionPass(*PR);
----------------
Comment not needed
================
Comment at: lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:207
@@ -202,2 +206,3 @@
addPass(createAMDGPUOpenCLImageTypeLoweringPass());
-
+ /*TODO:Uthkarsh - Integer Division*/
+ addPass(createAMDGPUIntegerDivisionPass(&getAMDGPUTargetMachine()));
----------------
Comments should never include names
================
Comment at: lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:208
@@ -204,1 +207,3 @@
+ /*TODO:Uthkarsh - Integer Division*/
+ addPass(createAMDGPUIntegerDivisionPass(&getAMDGPUTargetMachine()));
TargetPassConfig::addIRPasses();
----------------
I think this should be moved to addCodeGenPrepare after TargetPassConfig::addCodeGenPrepare()
http://reviews.llvm.org/D18072
More information about the llvm-commits
mailing list