<div dir="ltr">Ping!</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jul 16, 2020 at 2:19 PM Madhur Amilkanthwar via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">madhur13490 marked 2 inline comments as done.<br>
madhur13490 added inline comments.<br>
<br>
<br>
================<br>
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:3575<br>
+      HasPredicateCode = true;<br>
+      InsnMatcher.addPredicate<GenericInstructionPredicateMatcher>(Predicate);<br>
+    }<br>
----------------<br>
dsanders wrote:<br>
> dsanders wrote:<br>
> > madhur13490 wrote:<br>
> > > arsenm wrote:<br>
> > > > I think this should remain as the last predicate type added to the matcher, after everything else here. The ordering does matter (see D82331)<br>
> > > Well, then we have to remove all "continue" statements. Are we fine with it? I always thought that we'll use custom predicates for things which are *not* covered in other checks. I am fine with keeping it at the end, just looking for consequences. <br>
> > I'd be inclined to factor the builtins out into a function instead so they can return and then you resume at the code to check+add for custom predicates<br>
> Also, just to confirm I agree it should be added after the builtins. Otherwise your C++ can't assume anything covered by the builtins.<br>
Is it just for "assumption" or it would affect the functionality if custom predicates are added later? <br>
<br>
<br>
================<br>
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:3575<br>
+      HasPredicateCode = true;<br>
+      InsnMatcher.addPredicate<GenericInstructionPredicateMatcher>(Predicate);<br>
+    }<br>
----------------<br>
madhur13490 wrote:<br>
> dsanders wrote:<br>
> > dsanders wrote:<br>
> > > madhur13490 wrote:<br>
> > > > arsenm wrote:<br>
> > > > > I think this should remain as the last predicate type added to the matcher, after everything else here. The ordering does matter (see D82331)<br>
> > > > Well, then we have to remove all "continue" statements. Are we fine with it? I always thought that we'll use custom predicates for things which are *not* covered in other checks. I am fine with keeping it at the end, just looking for consequences. <br>
> > > I'd be inclined to factor the builtins out into a function instead so they can return and then you resume at the code to check+add for custom predicates<br>
> > Also, just to confirm I agree it should be added after the builtins. Otherwise your C++ can't assume anything covered by the builtins.<br>
> Is it just for "assumption" or it would affect the functionality if custom predicates are added later? <br>
Sure, I am up for refactoring, but can you please explain a bit more, how would you that refactoring look? I was more inclined to rewriting the loop which iterates over PredicateCalls and add matcher. One of the many ways in which we can refactor the loop is like <br>
<br>
```<br>
if (isAtomic())<br>
  if (isLoad())<br>
   add all matchers<br>
...<br>
``` <br>
but this may change the order in which matchers are added (which brings me back to the question - is the order important?). I can further outline this in a static function.<br>
But this is a bit of design question.<br>
<br>
So, I'd like to see what you're imagining in a form of pseudo code :)<br>
<br>
<br>
Repository:<br>
  rG LLVM Github Monorepo<br>
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D83034/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D83034/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D83034" rel="noreferrer" target="_blank">https://reviews.llvm.org/D83034</a><br>
<br>
<br>
<br>
</blockquote></div><br clear="all"><br>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div><i style="font-size:12.8px">Disclaimer: Views, concerns, thoughts, questions, ideas expressed in this mail are of my own and my employer has no take in it. </i><br></div><div>Thank You.<br>Madhur D. Amilkanthwar<br><br></div></div></div>