[PATCH] D158260: [GlobalISel] Add support for *_fpmode intrinsics

[Matt Arsenault via Phabricator via llvm-commits]

arsenm requested changes to this revision.
arsenm added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/include/llvm/Target/GenericOpcodes.td:1010
+// represented by side effect, because constrained intrinsics (which also acces
+// FP environment) use this way.
+
----------------
"use this way" doesn't read right


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:835
+  auto &Ctx = MF.getFunction().getContext();
+  auto &TLI = *MF.getSubtarget().getTargetLowering();
+
----------------
TLI is already available as a member of LegalizerHelper


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:843
+  MachinePointerInfo TempPtrInfo;
+  auto Temp = createStackTemporary(StateSize.divideCoefficientBy(8),
+                                   TempAlign, TempPtrInfo);
----------------
StateTy.getStoreSize


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:861
+  MIRBuilder.buildLoadInstr(TargetOpcode::G_LOAD, Dst, Temp, *MMO);
+
+  return LegalizerHelper::Legalized;
----------------
Need to erase the original instruction


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:875
+  auto &Ctx = MF.getFunction().getContext();
+  auto &TLI = *MF.getSubtarget().getTargetLowering();
+
----------------
TLI already a member


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:895
+  RTLIB::Libcall RTLibcall = getStateLibraryFunctionFor(MI, TLI);
+  return createLibcall(MIRBuilder, RTLibcall,
+                       CallLowering::ArgInfo({0}, Type::getVoidTy(Ctx), 0),
----------------
Didn't erase original instruction?


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:915-916
+  Type *StatePtrTy = PointerType::get(Ctx, AddrSpace);
+  Type *IntPtrTy = DL.getIntPtrType(Ctx, AddrSpace);
+  TypeSize PtrSize = IntPtrTy->getPrimitiveSizeInBits();
+  LLT MemTy = LLT::pointer(AddrSpace, PtrSize);
----------------
This is a roundabout way of doing DL.getPointerSizeInBits()


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:919-920
+  auto DefValue = MIRBuilder.buildConstant(LLT::scalar(PtrSize), -1LL);
+  DstOp Dest(MRI.createGenericVirtualRegister(MemTy));
+  MIRBuilder.buildIntToPtr(Dest, DefValue);
+
----------------
can fold these together with auto Dest = buildIntToPtr(MemTy)


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:923
+  RTLIB::Libcall RTLibcall = getStateLibraryFunctionFor(MI, TLI);
+  return createLibcall(MIRBuilder, RTLibcall,
+                       CallLowering::ArgInfo({0}, Type::getVoidTy(Ctx), 0),
----------------
Didn't erase original instruction?


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:972-973
   case TargetOpcode::G_INTRINSIC_ROUNDEVEN: {
+    LLT LLTy = MRI.getType(MI.getOperand(0).getReg());
+    unsigned Size = LLTy.getSizeInBits();
     Type *HLTy = getFloatTypeForLLT(Ctx, LLTy);
----------------
Unrelated?


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerInfo.cpp:123
+  // Skip null mutation.
+  if (!Mutation.second.isValid())
+    return true;
----------------
Were there really no other typeless G_* instructions before?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158260