[PATCH] D119621: [SanitizerCoverage] Add instrumentation callbacks for FP cmp instructions

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added inline comments.
Herald added a project: All.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_interface_internal.h:91
   void __sanitizer_cov_trace_cmp8();
   SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
+  void __sanitizer_cov_trace_cmp_fp2();
----------------
please rebase and clang-format the patch


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:967-983
       // __sanitizer_cov_trace_cmp((type_size << 32) | predicate, A0, A1);
       auto CallbackFunc = SanCovTraceCmpFunction[CallbackIdx];
-      bool FirstIsConst = isa<ConstantInt>(A0);
-      bool SecondIsConst = isa<ConstantInt>(A1);
+      bool FirstIsConst = isa<ConstantInt>(A0) || isa<ConstantFP>(A0);
+      bool SecondIsConst = isa<ConstantInt>(A1) || isa<ConstantFP>(A1);
       // If both are const, then we don't need such a comparison.
       if (FirstIsConst && SecondIsConst) continue;
       // If only one is const, then make it the first callback argument.
----------------
Can you please, in a separate patch, extract utility method:
void InsertCallbackForTraceForCmp(CallbackIdx, CallbackArgsTy, A0, A1...

And than in the D119621
 you can do


```
if (isa<ICmpInst>(I)) {
  ...
  InsertCallbackForTraceForCmp
} else if isa<FCmpInst>(I)) {
  ...
  InsertCallbackForTraceForCmp
}
```

Please link them into stack using "edit related revisions" in the top of the review


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

https://reviews.llvm.org/D119621