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

[Simon Moll via Phabricator via llvm-commits]

simoll marked 5 inline comments as done.
simoll added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:302
+      case Intrinsic::vp_srem:
+        return Instruction::SRem;
+      }
----------------
jdoerfert wrote:
> I (started to) like generating these constructs. It will make additions later easier and less (copy&paste) error prone.
> If you put the mapping in a file with a macro
> ```
> #ifndef OPCODE2VPINTRINSIC
> #define OPCODE2VPINTRINSIC(Opcode, VPIntrinsic)
> #endif
> 
> OPCODE2VPINTRINSIC(And, vp_and)
> ...
> 
> #undef OPCODE2VPINTRINSIC
> ```
> 
> you can replace all these long lists with codes like:
> 
> ```
> #define OPCODE2VPINTRINSIC(Opcode, VPIntrinsic) case Instruction::Opcode: return Intrinsic::VPIntrinsic;
> #include ".../VPLanInstructions.def"
> ```
> 
You are preaching to the choir here - i made it that way to stay closer to the other intrinsic classes. Update will have a macro file similar to `llvm/IR/Instruction.def`.


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:1172
+
+}
+
----------------
jdoerfert wrote:
> I guess you can add readnone as an attribute, also nosync and nofree please.
The intrinsics are `readnone` already. `nosync` seems sensible.


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:216
+
+bool VPIntrinsic::canIgnoreVectorLengthParam() const {
+  auto StaticVL = getStaticVectorLength();
----------------
samparker wrote:
> Are you flexible on this..? For Arm's MVE, it would be useful for us to be able to use a vector length even though we have a fixed vector width. We are trying to use the vectorizers tail folding ability and it could be really nice to use the vector length instead of performing a vector icmp for active lanes. This could free up the 'mask' for other conditions within the loop without having to compare conditions (which will be expensive for us).
Flexible in what sense? The vector length param should work with scalable types already.

To clarify: this returns whether the vector length value for this instance of the intrinsic does not have a masking effect on the lanes. This happens when the vector length parameter is a constant that is larger than the length of the vector type (or if the MSB is set). I'll hoist the test in l225 to l240 up so you can "turn off" the vector length param also for SVE/MVE ( by passing `i32 -1` as the vlen).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69891