[PATCH] D68233: [FPEnv] [WIP] Verify strictfp attribute correctness

[Matt Arsenault via Phabricator via llvm-commits]

arsenm requested changes to this revision.
arsenm added inline comments.
This revision now requires changes to proceed.
Herald added a subscriber: StephenFan.
Herald added a project: All.


================
Comment at: llvm/lib/IR/Verifier.cpp:3174
+  AttributeSet CallAttrs = Attrs.getFnAttributes();
+  Assert (ContainingF->hasFnAttribute(Attribute::StrictFP) ==
+          CallAttrs.hasAttribute(Attribute::StrictFP),
----------------
I think these Asserts were renamed to Check


================
Comment at: llvm/lib/IR/Verifier.cpp:3175
+  Assert (ContainingF->hasFnAttribute(Attribute::StrictFP) ==
+          CallAttrs.hasAttribute(Attribute::StrictFP),
+            "Functions and their contained calls and invokes must match "
----------------
Shouldn't require the callsite attributes to match if the underlying declaration is strictfp, your test doesn't cover that case


================
Comment at: llvm/test/Verifier/fp-intrinsics.ll:2-9
 ; RUN: sed -e s/.T2:// %s | not opt -verify -disable-output 2>&1 | FileCheck --check-prefix=CHECK2 %s
 ; RUN: sed -e s/.T3:// %s | not opt -verify -disable-output 2>&1 | FileCheck --check-prefix=CHECK3 %s
 ; RUN: sed -e s/.T4:// %s | not opt -verify -disable-output 2>&1 | FileCheck --check-prefix=CHECK4 %s
 ; RUN: sed -e s/.T5:// %s | not opt -verify -disable-output 2>&1 | FileCheck --check-prefix=CHECK5 %s
+; RUN: sed -e s/.T6:// %s | not opt -verify -disable-output 2>&1 | FileCheck --check-prefix=CHECK6 %s
+; RUN: sed -e s/.T7:// %s | not opt -verify -disable-output 2>&1 | FileCheck --check-prefix=CHECK7 %s
+; RUN: sed -e s/.T7I:// %s | not opt -verify -disable-output 2>&1 | FileCheck --check-prefix=CHECK7I %s
----------------
I don't understand the sed usage of this test. Verifier checks can all coexist in the same module. This test only needs a single run line with no sed


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

https://reviews.llvm.org/D68233