[PATCH] D69891: [VP,Integer,#1] Vector-predicated integer intrinsics

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:1172
+
+}
+
----------------
sstefan1 wrote:
> simoll wrote:
> > jdoerfert wrote:
> > > simoll wrote:
> > > > jdoerfert wrote:
> > > > > I guess you can add readnone as an attribute, also nosync and nofree please.
> > > > The intrinsics are `readnone` already. `nosync` seems sensible.
> > > I asked @sstefan1 to add nosync and nofree to the td file and other places needed to use them. That should make your changes more concise.
> > > 
> > > ---
> > > 
> > > I would have argued we could even do the `#inline` trick here, or maybe some .td magic, to avoid this replication, but I will not force anything.
> > > I would have argued we could even do the `#inline` trick here, or maybe some .td magic, to avoid this replication, but I will not force anything.
> > 
> > I agree but refactoring `Intrinsics.td` is out of scope for this patch.
> I kind of started some of this in D65377 and the approach will try and propose is an opt-out list.
> 
> If I understood correctly I should try to do that part first (and propose). @jdoerfert correct me if I'm wrong.
> 
> Hopefully I'll start this today.
@simoll Agreed. This is out of scope.
@sstefan1 I think your plan sounds good.


================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:1
+//===-- llvm/Instruction.def - File that describes Instructions -*- C++ -*-===//
+//
----------------
jdoerfert wrote:
> Nit: copy&past
now too long and wrapped :(
No need for `llvm/`, which is confusing as it lives in `llvm/IR`.
Also "Instructions" might be a bit to broad? Idk, maybe: "vector intrinsic descriptions"


================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:27
+#define HANDLE_VP_TO_OC(VPID,OC)
+#endif
+
----------------
simoll wrote:
> jdoerfert wrote:
> > To be honest, I would have made the Opcode for vp intrinsics of existing functions `VP_EXISTING` (or similar) and used a single macro definition here, assuming all vp intrinsics so far have a unique corresponding scalar version.
> > These are two unrelated points you can consider (I won't force either).
> > 
> > (If you device to do both changes, you can even get away by only defining the existing opcode in the intrinsic "macro definition").
> > assuming all vp intrinsics so far have a unique corresponding scalar version.
> They don't. That's why it's separate. There will be more cases for constrained fp, reduction and memory ops.
> 
I see. Though, even if later intriniscs do not correspond to scalar instructions, the changes would still be possible. I admit, at this point it is a question if you like:
```
REGISTER_VP_INTRINSIC(vp_add, 2, 3)
HANDLE_VP_TO_OC(vp_add, Add)
REGISTER_VP_INTRINSIC(vp_red, 2, 3)
```
better or worse than
```
REGISTER_VP_FROM_SCALAR_INTRINSIC(Add, 2, 3)
REGISTER_VP_INTRINSIC(Red, 2, 3)
```
with the appropriate definition of the above:
```
#define REGISTER_VP_FROM_SCALAR_INTRINSIC(OC, MP, VP) \
  REGISTER_VP_INTRINSIC(OC, MP, VP) \
  HANDLE_VP_TO_OC(vp_##OC, OC)
```


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:244
+  return GetStaticVectorLengthOfType(VPMask->getType());
+}
+
----------------
simoll wrote:
> jdoerfert wrote:
> > Does it make sense to have this corner case in here or should we just make it an `Optional<int64_t>` and call it a day?
> > 
> > You can also remove the lambda if you move the code, except there is a reason to keep it.
> Actually, the excessive vector length case should `return None` to imply that the static vector length could not be inferred.
> 
> Btw, later patches will reuse the lambda.
Fine with me.


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:306
+  default:
+    return Instruction::Call;
+
----------------
simoll wrote:
> jdoerfert wrote:
> > I doubt this is a sensible default. I doubt one should return anything for a non-vp intrinsic.
> If there is no functional IR opcode this is still a call to an intrinsic function. It's natural to return `Call` in this case.
not really convinced but OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69891