[PATCH] D43515: More math intrinsics for conservative math handling

[Kit Barton via Phabricator via llvm-commits]

kbarton added a comment.

A few minor comments about commoning asserts and using dyn_cast instead of cast<>.
Aside from that, I think this looks good. That said, I'm by no means an expert in this area so don't feel I'm qualified to give a final approval to commit.



================
Comment at: lib/IR/Verifier.cpp:4578
+    Value *Operand = FPI.getArgOperand(0);
+    uint64_t NumSrcElem = 0;
+    if (Operand->getType()->isVectorTy()) {
----------------
Could you use the isFPOrFPVectorTy here to check both conditions, and then remove the assert in the else below?


================
Comment at: lib/IR/Verifier.cpp:4579
+    uint64_t NumSrcElem = 0;
+    if (Operand->getType()->isVectorTy()) {
+      auto *OperandT = cast<VectorType>(Operand->getType());
----------------
Can you use a dyn_cast here instead?
if (auto *OperandT = dyn_cast<VectorType>(Operand->getType())) {
  do vector stuff
}
  


================
Comment at: lib/IR/Verifier.cpp:4594
+           "Intrinsic first argument and result disagree on vector use",
+           &FPI);
+    if (Operand->getType()->isVectorTy()) {
----------------
Similarly, could you use isIntOrIntVectorTy here and remove the assert in the if/else below?


================
Comment at: lib/IR/Verifier.cpp:4618
+    Value *Operand = FPI.getArgOperand(0);
+    uint64_t NumSrcElem = 0;
+    if (Operand->getType()->isVectorTy()) {
----------------
same comment about commoning the asserts here. 


================
Comment at: lib/IR/Verifier.cpp:4662
+  if (HasRoundingMD) {
+    int RoundingIdx = (HasExceptionMD ? NumOperands - 2 : NumOperands - 1);
+    Assert(isa<MetadataAsValue>(FPI.getArgOperand(RoundingIdx)),
----------------
It looks like getArgOperand expects an unsigned. Is there a specific reason you are using an int here?
If it's possible that RoundingIdx is negative, then you should probably add an assert here before passing it to getArgOperand. 


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

https://reviews.llvm.org/D43515