[PATCH] D112791: [IR] Merge createReplacementInstr into ConstantExpr::getAsInstruction

[Jay Foad via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG1b758925adf6: [IR] Merge createReplacementInstr into 
ConstantExpr::getAsInstruction (authored by foad).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112791

Files:
  clang/lib/CodeGen/CGCUDANV.cpp
  llvm/include/llvm/IR/Constants.h
  llvm/lib/IR/Constants.cpp
  llvm/lib/IR/ReplaceConstant.cpp
  llvm/lib/Target/XCore/XCoreLowerThreadLocal.cpp
  llvm/lib/Transforms/IPO/GlobalOpt.cpp
  llvm/lib/Transforms/Scalar/ConstantHoisting.cpp

Index: llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
===
--- llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
+++ llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
@@ -819,10 +819,9 @@
 
 // Aside from constant GEPs, only constant cast expressions are collected.
 assert(ConstExpr->isCast() && "ConstExpr should be a cast");
-Instruction *ConstExprInst = ConstExpr->getAsInstruction();
+Instruction *ConstExprInst = ConstExpr->getAsInstruction(
+findMatInsertPt(ConstUser.Inst, ConstUser.OpndIdx));
 ConstExprInst->setOperand(0, Mat);
-ConstExprInst->insertBefore(findMatInsertPt(ConstUser.Inst,
-ConstUser.OpndIdx));
 
 // Use the same debug location as the instruction we are about to update.
 ConstExprInst->setDebugLoc(ConstUser.Inst->getDebugLoc());
Index: llvm/lib/Transforms/IPO/GlobalOpt.cpp
===
--- llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -1490,8 +1490,7 @@
 append_range(UUsers, U->users());
 for (auto *UU : UUsers) {
   Instruction *UI = cast(UU);
-  Instruction *NewU = U->getAsInstruction();
-  NewU->insertBefore(UI);
+  Instruction *NewU = U->getAsInstruction(UI);
   UI->replaceUsesOfWith(U, NewU);
 }
 // We've replaced all the uses, so destroy the constant. (destroyConstant
Index: llvm/lib/Target/XCore/XCoreLowerThreadLocal.cpp
===
--- llvm/lib/Target/XCore/XCoreLowerThreadLocal.cpp
+++ llvm/lib/Target/XCore/XCoreLowerThreadLocal.cpp
@@ -21,7 +21,6 @@
 #include "llvm/IR/IntrinsicsXCore.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/NoFolder.h"
-#include "llvm/IR/ReplaceConstant.h"
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/CommandLine.h"
@@ -90,11 +89,11 @@
   if (PredBB->getTerminator()->getNumSuccessors() > 1)
 PredBB = SplitEdge(PredBB, PN->getParent());
   Instruction *InsertPos = PredBB->getTerminator();
-  Instruction *NewInst = createReplacementInstr(CE, InsertPos);
+  Instruction *NewInst = CE->getAsInstruction(InsertPos);
   PN->setOperand(I, NewInst);
 }
 } else if (Instruction *Instr = dyn_cast(WU)) {
-  Instruction *NewInst = createReplacementInstr(CE, Instr);
+  Instruction *NewInst = CE->getAsInstruction(Instr);
   Instr->replaceUsesOfWith(CE, NewInst);
 } else {
   ConstantExpr *CExpr = dyn_cast(WU);
@@ -103,7 +102,7 @@
 }
   }
   } while (CE->hasNUsesOrMore(1)); // We need to check because a recursive
-  // sibling may have used 'CE' when createReplacementInstr was called.
+  // sibling may have used 'CE' when getAsInstruction was called.
   CE->destroyConstant();
   return true;
 }
Index: llvm/lib/IR/ReplaceConstant.cpp
===
--- llvm/lib/IR/ReplaceConstant.cpp
+++ llvm/lib/IR/ReplaceConstant.cpp
@@ -20,9 +20,7 @@
 // Replace a constant expression by instructions with equivalent operations at
 // a specified location.
 Instruction *createReplacementInstr(ConstantExpr *CE, Instruction *Instr) {
-  auto *CEInstr = CE->getAsInstruction();
-  CEInstr->insertBefore(Instr);
-  return CEInstr;
+  return CE->getAsInstruction(Instr);
 }
 
 void convertConstantExprsToInstructions(Instruction *I, ConstantExpr *CE,
@@ -63,8 +61,7 @@
   for (auto *CE : Path) {
 if (!Visited.insert(CE).second)
   continue;
-auto *NI = CE->getAsInstruction();
-NI->insertBefore(BI);
+auto *NI = CE->getAsInstruction(BI);
 II->replaceUsesOfWith(CE, NI);
 CE->removeDeadConstantUsers();
 BI = II = NI;
Index: llvm/lib/IR/Constants.cpp
===
--- llvm/lib/IR/Constants.cpp
+++ llvm/lib/IR/Constants.cpp
@@ -3492,7 +3492,7 @@
   NewOps, this, From, To, NumUpdated, OperandNo);
 }
 
-Instruction *ConstantExpr::getAsInstruction() const {
+Instruction *ConstantExpr::getAsInstruction(Instruction *InsertBefore) const {
   SmallVector 

[PATCH] D112791: [IR] Merge createReplacementInstr into ConstantExpr::getAsInstruction

[Jay Foad via Phabricator via cfe-commits]

foad added inline comments.



Comment at: llvm/include/llvm/IR/Constants.h:1317
   /// would make it harder to remove ConstantExprs altogether.
-  Instruction *getAsInstruction() const;
+  Instruction *getAsInstruction(Instruction *InsertBefore = nullptr) const;
 

yaxunl wrote:
> Can you add a comment about the insertion location when 'InsertBefore' is 
> nullptr? Thanks.
Done, although there are loads of InsertBefore arguments in Instructions.h that 
all work the same way, and no comments explaining them :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112791

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112791: [IR] Merge createReplacementInstr into ConstantExpr::getAsInstruction

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl accepted this revision.
yaxunl added a comment.

LGTM. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112791

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112791: [IR] Merge createReplacementInstr into ConstantExpr::getAsInstruction

[Jay Foad via Phabricator via cfe-commits]

foad updated this revision to Diff 383341.
foad added a comment.

Add comment about InsertBefore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112791

Files:
  clang/lib/CodeGen/CGCUDANV.cpp
  llvm/include/llvm/IR/Constants.h
  llvm/lib/IR/Constants.cpp
  llvm/lib/IR/ReplaceConstant.cpp
  llvm/lib/Target/XCore/XCoreLowerThreadLocal.cpp
  llvm/lib/Transforms/IPO/GlobalOpt.cpp
  llvm/lib/Transforms/Scalar/ConstantHoisting.cpp

Index: llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
===
--- llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
+++ llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
@@ -819,10 +819,9 @@
 
 // Aside from constant GEPs, only constant cast expressions are collected.
 assert(ConstExpr->isCast() && "ConstExpr should be a cast");
-Instruction *ConstExprInst = ConstExpr->getAsInstruction();
+Instruction *ConstExprInst = ConstExpr->getAsInstruction(
+findMatInsertPt(ConstUser.Inst, ConstUser.OpndIdx));
 ConstExprInst->setOperand(0, Mat);
-ConstExprInst->insertBefore(findMatInsertPt(ConstUser.Inst,
-ConstUser.OpndIdx));
 
 // Use the same debug location as the instruction we are about to update.
 ConstExprInst->setDebugLoc(ConstUser.Inst->getDebugLoc());
Index: llvm/lib/Transforms/IPO/GlobalOpt.cpp
===
--- llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -1490,8 +1490,7 @@
 append_range(UUsers, U->users());
 for (auto *UU : UUsers) {
   Instruction *UI = cast(UU);
-  Instruction *NewU = U->getAsInstruction();
-  NewU->insertBefore(UI);
+  Instruction *NewU = U->getAsInstruction(UI);
   UI->replaceUsesOfWith(U, NewU);
 }
 // We've replaced all the uses, so destroy the constant. (destroyConstant
Index: llvm/lib/Target/XCore/XCoreLowerThreadLocal.cpp
===
--- llvm/lib/Target/XCore/XCoreLowerThreadLocal.cpp
+++ llvm/lib/Target/XCore/XCoreLowerThreadLocal.cpp
@@ -21,7 +21,6 @@
 #include "llvm/IR/IntrinsicsXCore.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/NoFolder.h"
-#include "llvm/IR/ReplaceConstant.h"
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/CommandLine.h"
@@ -90,11 +89,11 @@
   if (PredBB->getTerminator()->getNumSuccessors() > 1)
 PredBB = SplitEdge(PredBB, PN->getParent());
   Instruction *InsertPos = PredBB->getTerminator();
-  Instruction *NewInst = createReplacementInstr(CE, InsertPos);
+  Instruction *NewInst = CE->getAsInstruction(InsertPos);
   PN->setOperand(I, NewInst);
 }
 } else if (Instruction *Instr = dyn_cast(WU)) {
-  Instruction *NewInst = createReplacementInstr(CE, Instr);
+  Instruction *NewInst = CE->getAsInstruction(Instr);
   Instr->replaceUsesOfWith(CE, NewInst);
 } else {
   ConstantExpr *CExpr = dyn_cast(WU);
@@ -103,7 +102,7 @@
 }
   }
   } while (CE->hasNUsesOrMore(1)); // We need to check because a recursive
-  // sibling may have used 'CE' when createReplacementInstr was called.
+  // sibling may have used 'CE' when getAsInstruction was called.
   CE->destroyConstant();
   return true;
 }
Index: llvm/lib/IR/ReplaceConstant.cpp
===
--- llvm/lib/IR/ReplaceConstant.cpp
+++ llvm/lib/IR/ReplaceConstant.cpp
@@ -20,9 +20,7 @@
 // Replace a constant expression by instructions with equivalent operations at
 // a specified location.
 Instruction *createReplacementInstr(ConstantExpr *CE, Instruction *Instr) {
-  auto *CEInstr = CE->getAsInstruction();
-  CEInstr->insertBefore(Instr);
-  return CEInstr;
+  return CE->getAsInstruction(Instr);
 }
 
 void convertConstantExprsToInstructions(Instruction *I, ConstantExpr *CE,
@@ -63,8 +61,7 @@
   for (auto *CE : Path) {
 if (!Visited.insert(CE).second)
   continue;
-auto *NI = CE->getAsInstruction();
-NI->insertBefore(BI);
+auto *NI = CE->getAsInstruction(BI);
 II->replaceUsesOfWith(CE, NI);
 CE->removeDeadConstantUsers();
 BI = II = NI;
Index: llvm/lib/IR/Constants.cpp
===
--- llvm/lib/IR/Constants.cpp
+++ llvm/lib/IR/Constants.cpp
@@ -3492,7 +3492,7 @@
   NewOps, this, From, To, NumUpdated, OperandNo);
 }
 
-Instruction *ConstantExpr::getAsInstruction() const {
+Instruction *ConstantExpr::getAsInstruction(Instruction *InsertBefore) const {
   SmallVector ValueOperands(operands());
   ArrayRef Ops(ValueOperands);
 
@@ -3510,40 +3510,43 @@
   case Instruction::IntToPtr:
   case Instruction::BitCast:
   case 

[PATCH] D112791: [IR] Merge createReplacementInstr into ConstantExpr::getAsInstruction

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.



Comment at: llvm/include/llvm/IR/Constants.h:1317
   /// would make it harder to remove ConstantExprs altogether.
-  Instruction *getAsInstruction() const;
+  Instruction *getAsInstruction(Instruction *InsertBefore = nullptr) const;
 

Can you add a comment about the insertion location when 'InsertBefore' is 
nullptr? Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112791

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112791: [IR] Merge createReplacementInstr into ConstantExpr::getAsInstruction

[Mahesha S via Phabricator via cfe-commits]

hsmhsm accepted this revision.
hsmhsm added a comment.
This revision is now accepted and ready to land.

Thanks for this clean-up patch. Looks good to me. However, please wait for some 
time if in case other reviewers have any comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112791

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112791: [IR] Merge createReplacementInstr into ConstantExpr::getAsInstruction

[Jay Foad via Phabricator via cfe-commits]

foad created this revision.
Herald added subscribers: ormris, dexonsmith, hiraditya.
foad requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

createReplacementInstr was a trivial wrapper around
ConstantExpr::getAsInstruction, which also inserted the new instruction
into a basic block. Implement this directly in getAsInstruction by
adding an InsertBefore parameter and change all callers to use it. NFC.

A follow-up patch will remove createReplacementInstr.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112791

Files:
  clang/lib/CodeGen/CGCUDANV.cpp
  llvm/include/llvm/IR/Constants.h
  llvm/lib/IR/Constants.cpp
  llvm/lib/IR/ReplaceConstant.cpp
  llvm/lib/Target/XCore/XCoreLowerThreadLocal.cpp
  llvm/lib/Transforms/IPO/GlobalOpt.cpp
  llvm/lib/Transforms/Scalar/ConstantHoisting.cpp

Index: llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
===
--- llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
+++ llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
@@ -819,10 +819,9 @@
 
 // Aside from constant GEPs, only constant cast expressions are collected.
 assert(ConstExpr->isCast() && "ConstExpr should be a cast");
-Instruction *ConstExprInst = ConstExpr->getAsInstruction();
+Instruction *ConstExprInst = ConstExpr->getAsInstruction(
+findMatInsertPt(ConstUser.Inst, ConstUser.OpndIdx));
 ConstExprInst->setOperand(0, Mat);
-ConstExprInst->insertBefore(findMatInsertPt(ConstUser.Inst,
-ConstUser.OpndIdx));
 
 // Use the same debug location as the instruction we are about to update.
 ConstExprInst->setDebugLoc(ConstUser.Inst->getDebugLoc());
Index: llvm/lib/Transforms/IPO/GlobalOpt.cpp
===
--- llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -1490,8 +1490,7 @@
 append_range(UUsers, U->users());
 for (auto *UU : UUsers) {
   Instruction *UI = cast(UU);
-  Instruction *NewU = U->getAsInstruction();
-  NewU->insertBefore(UI);
+  Instruction *NewU = U->getAsInstruction(UI);
   UI->replaceUsesOfWith(U, NewU);
 }
 // We've replaced all the uses, so destroy the constant. (destroyConstant
Index: llvm/lib/Target/XCore/XCoreLowerThreadLocal.cpp
===
--- llvm/lib/Target/XCore/XCoreLowerThreadLocal.cpp
+++ llvm/lib/Target/XCore/XCoreLowerThreadLocal.cpp
@@ -21,7 +21,6 @@
 #include "llvm/IR/IntrinsicsXCore.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/NoFolder.h"
-#include "llvm/IR/ReplaceConstant.h"
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/CommandLine.h"
@@ -90,11 +89,11 @@
   if (PredBB->getTerminator()->getNumSuccessors() > 1)
 PredBB = SplitEdge(PredBB, PN->getParent());
   Instruction *InsertPos = PredBB->getTerminator();
-  Instruction *NewInst = createReplacementInstr(CE, InsertPos);
+  Instruction *NewInst = CE->getAsInstruction(InsertPos);
   PN->setOperand(I, NewInst);
 }
 } else if (Instruction *Instr = dyn_cast(WU)) {
-  Instruction *NewInst = createReplacementInstr(CE, Instr);
+  Instruction *NewInst = CE->getAsInstruction(Instr);
   Instr->replaceUsesOfWith(CE, NewInst);
 } else {
   ConstantExpr *CExpr = dyn_cast(WU);
@@ -103,7 +102,7 @@
 }
   }
   } while (CE->hasNUsesOrMore(1)); // We need to check because a recursive
-  // sibling may have used 'CE' when createReplacementInstr was called.
+  // sibling may have used 'CE' when getAsInstruction was called.
   CE->destroyConstant();
   return true;
 }
Index: llvm/lib/IR/ReplaceConstant.cpp
===
--- llvm/lib/IR/ReplaceConstant.cpp
+++ llvm/lib/IR/ReplaceConstant.cpp
@@ -20,9 +20,7 @@
 // Replace a constant expression by instructions with equivalent operations at
 // a specified location.
 Instruction *createReplacementInstr(ConstantExpr *CE, Instruction *Instr) {
-  auto *CEInstr = CE->getAsInstruction();
-  CEInstr->insertBefore(Instr);
-  return CEInstr;
+  return CE->getAsInstruction(Instr);
 }
 
 void convertConstantExprsToInstructions(Instruction *I, ConstantExpr *CE,
@@ -63,8 +61,7 @@
   for (auto *CE : Path) {
 if (!Visited.insert(CE).second)
   continue;
-auto *NI = CE->getAsInstruction();
-NI->insertBefore(BI);
+auto *NI = CE->getAsInstruction(BI);
 II->replaceUsesOfWith(CE, NI);
 CE->removeDeadConstantUsers();
 BI = II = NI;
Index: llvm/lib/IR/Constants.cpp
===
--- llvm/lib/IR/Constants.cpp
+++ llvm/lib/IR/Constants.cpp
@@ -3492,7 +3492,7 @@