[PATCH] D158147: [RFC][GlobalISel] convergence control tokens and intrinsics

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2462
+    auto *Token = Bundle->Inputs[0].get();
+    ConvergenceCtrlToken = getOrCreateVReg(*Token);
+  }
----------------
sameerds wrote:
> arsenm wrote:
> > Just directly create the token vreg 
> I didn't understand. In the LLVM IR, this is a token use, and the token definition was elsewhere. We still need to find that existing register, right?
Yes, you still need to update the map entry. But you don't need to go through the full getOrCreateVregs with all of the type handling 


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:122
 
-  if (isa<GIntrinsic>(MI))
+  if (auto *GI = dyn_cast<GIntrinsic>(&MI)) {
+    auto ID = GI->getIntrinsicID();
----------------
sameerds wrote:
> arsenm wrote:
> > Wait, why is this here? Generic intrinsics shouldn't reach the legalizer, they should go through G_* instruction wrappers. If your intention was to default legalize to erase, those should be just in the default lower action
> I don't understand, mostly because I am quite unfamiliar with the backend. But in GlobalISel.cpp, the legalizer comes before selection. So this definitely hits the legalizer? By "default lower action", you mean somewhere in `InstructionSelect::runOnMachineFunction()` before it calls the target-specific virtual function `InstructionSelector::select()`?
The legalizer shouldn't need to specially intercept intrinsics. A generic intrinsic should not pass the IRTranslator. These should be swapped to new G_* instructions which the ordinary legalizer can express rules. The default lower action for them can be to just erase (or we can let these pass to selection)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158147