[PATCH] D92086: Generalized PatternMatch & InstSimplify

[Simon Moll via Phabricator via llvm-commits]

simoll added a comment.

Some comments/thoughts inline.
In the meantime, upstream got new fp patterns that consider the fp environment. This should work just fine with this approach (see my comment on the `consider` function - we may even DCE fp-environment-aware-patterns for Traits that support fp but do not care about the fp env).



================
Comment at: llvm/include/llvm/IR/Traits/Traits.h:267
+  static bool consider(const Value *V) {
+    return isa<ConstrainedFPIntrinsic>(V);
+  }
----------------
Note that we can DCE pattern-match paths in the trait-instantiated pattern rewrites, if we make this function more transparent to the compiler.

Not all pattern rewrites make sense for all traits. Eg, the Constrained FP trait does not care about anything but fp arithmetic. If we turn `consider` into a switch over opcodes right in this header file, say, the compiler may have a chance to detect dead pattern matching paths in the code. The result would be that when eg `simplifyAddInst` is instantiated for the CFPTrait, the function would be almost empty (since all non-fp opcodes are rejected - and the compiler can (hopefully) discard the int arithmetic patterns for the CFPTrait).



================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:3694
 
-      if (LHS_CR.icmp(CmpInst::getInversePredicate(Pred), RHS_CR))
+      auto InversedSatisfied_CR = ConstantRange::makeSatisfyingICmpRegion(
+          CmpInst::getInversePredicate(Pred), RHS_CR);
----------------
Ignore. Stale code kept during rebase.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4261
   return ConstantFoldInstOperands(I, ConstOps, Q.DL, Q.TLI);
+
+  // If all operands are constant after substituting Op for RepOp then we can
----------------
Ignore. Stale code kept during rebase.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:2253
+  // FIXME: Existing trait-unaware patterns should keep working without code changes.
+  template <typename ITy, typename Trait=DefaultTrait> bool match(ITy *V, MatcherContext<Trait> &MContext) {
+    return L->isLoopInvariant(V) && SubPattern.match(V, MContext);
----------------
I am not happy about this change. The PatternMatch abstractions should be entirely transparent to external code. External code would be code outside of PatternMatch or InstSimplify/Combine.

One way to get rid of this change in external code, is to understand that the issue only arises when "our" code calls into "external" code expecting there to be a `match` function with trait parameter - but never when "external" patterns call into "our" patterns because then everything will default to calling `match` functions without trait parameter.


================
Comment at: llvm/test/Transforms/InstSimplify/fast-math-strictfp.ll:96
 ;
   %negx = fneg <2 x float> %x
   %r = call nnan <2 x float> @llvm.experimental.constrained.fadd.v2f32(<2 x float> %x, <2 x float> %negx, metadata !"round.tonearest", metadata !"fpexcept.ignore") #0
----------------
This test is actually illformed, [according to the LangRef](https://llvm.org/docs/LangRef.html#constrainedfp):

"If any FP operation in a function is constrained then they all must be constrained. This is required for correct LLVM IR. "


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92086