[PATCH] D44424: [InstCombine] peek through unsigned FP casts for zero-equality compares (PR36682)

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:4516
+      return new ICmpInst(Pred, X, ConstantInt::getNullValue(X->getType()));
+  }
+
----------------
spatel wrote:
> lebedev.ri wrote:
> > spatel wrote:
> > > Don't need braces for the single-line 'if' clause.
> > It occupies more than one line though.
> > But ok, if you insist.
> It's a fuzzy rule, so I was just going by the formatting of the code above this.
Ok, consistency reason is good.


================
Comment at: test/Transforms/InstCombine/cast-unsigned-icmp-eqcmp-0.ll:4
 
 ; target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 
----------------
spatel wrote:
> lebedev.ri wrote:
> > spatel wrote:
> > > Remove unnecessary comment line.
> > For future reference, could you please clarify *when* `target datalayout` //should// be specified in tests?
> Sure - it should be specified when it affects the outcome. For instcombine, this would be when endianness or type legality is used to determine if/how a transform fires. So you'd see some query of the datalayout in the code.
> 
> Note that in this case, the line is commented out, so it definitely doesn't add anything to the test....unless I've missed some reason for it to be specified as a comment?
> Note that in this case, the line is commented out, so it definitely doesn't add anything to the test....

Oops, i kinda missed that :)

> unless I've missed some reason for it to be specified as a comment?

Not that i know of.

Ok, will drop it.


Repository:
  rL LLVM

https://reviews.llvm.org/D44424