[PATCH] D48017: AMDGPU: Select MIMG instructions manually in SITargetLowering

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added inline comments.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:4505-4506
+
+      MVT StoreVT = VData.getSimpleValueType();
+      if (StoreVT == MVT::f16 || StoreVT == MVT::v2f16 || StoreVT == MVT::v4f16) {
+        if (Subtarget->getGeneration() < SISubtarget::VOLCANIC_ISLANDS ||
----------------
arsenm wrote:
> .ScalarType() == s16
Done.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:4507
+      if (StoreVT == MVT::f16 || StoreVT == MVT::v2f16 || StoreVT == MVT::v4f16) {
+        if (Subtarget->getGeneration() < SISubtarget::VOLCANIC_ISLANDS ||
+            !BaseOpcode->HasD16)
----------------
arsenm wrote:
> I think we already have a hasD16 feature?
I don't see one.  There's a hasUnpackedD16VMem(), but that just says whether `D16` is unpacked or packed (if it exists).

FWIW, I'm not a fan of having explicit feature flags for features that are already clearly delineated by hardware generations, which is the case for `D16`.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:4527
+        if (LoadVT.isVector() && (Unpacked || !isTypeLegal(LoadVT))) {
+          // TODO simplify this for the packed case once v4f16 is legal
+          if (LoadVT == MVT::v2f16) {
----------------
arsenm wrote:
> I'm going to be committing that patch soon so might as well just wait for that
Sure, will do. There's a few more changes to come anyway, and I want to submit them all at once to reduce merge conflicts with our internal branches.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:4550
+      SDValue Undef = DAG.getUNDEF(Op.getValueType());
+      if (isa<MemSDNode>(Op))
+        return DAG.getMergeValues({ Undef, Op.getOperand(0) }, DL);
----------------
arsenm wrote:
> Isn't this always the case?
No. getlod and getresinfo don't access memory and are INTRINSIC_WO_CHAIN.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:4565-4566
+
+  SDValue True = DAG.getTargetConstant(1, DL, MVT::i32);
+  SDValue False = DAG.getTargetConstant(0, DL, MVT::i32);
+  unsigned CtrlIdx; // Index of texfailctrl argument
----------------
arsenm wrote:
> i1?
Good point, changed. It doesn't seem like any downstream users care...


Repository:
  rL LLVM

https://reviews.llvm.org/D48017