[PATCH] D69281: [FPEnv][WIP] Constrained FCmp intrinsics

Ulrich Weigand via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Nov 4 09:04:21 PST 2019


uweigand added a comment.

In D69281#1723640 <https://reviews.llvm.org/D69281#1723640>, @cameron.mcinally wrote:

> > I did not actually require any TableGen changes, but was able to represent the overloaded intrinsic types using existing mechanisms. This solution looks correct to me, but if I'm missing anything here, please let me know ...
>
> That sounds right. `LLVMScalarOrSameVectorWidth` did not exist when D54649 <https://reviews.llvm.org/D54649> was proposed, IINM.


OK, thanks for your review!

>> This adds a full set of compare intrinsics for all comparison codes.
> 
> There's really no reasonable alternative, correct? I believe that negating `<` and `>` would get messy wrt signals when a NaN is present. I don't think that savings is worth the effort (e.g. readability).

Agreed.

>> The X86 back-end changes are incomplete, they're right now the bare minimum to keep Cameron's two test cases working.
> 
> I'd be ok with leaving these changes to a later patch. Your call...

OK, I'm dropping the X86-related changes for now.

Due to the switch to the github monorepo, it seems all the inline comments disappeared -- sorry for that.

Anyway, I believe I've anyway addressed them all:

> lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7073
>  These DAG.getNode changes could probably be broken out into a standalone Diff.

Agreed.  Since this is just a simple refactoring, I've checked this in as  664f84e246478db82be2871f36fd1a523d9f2731 <https://reviews.llvm.org/rG664f84e246478db82be2871f36fd1a523d9f2731>.

> lib/Target/SystemZ/SystemZISelLowering.cpp:2171
>  This is probably worthy of a comment.

Agreed.  Added comment.

> lib/Target/SystemZ/SystemZISelLowering.cpp:2655
>  I notice the static_cast<SystemZISD::NodeType> has been dropped. Is this ok?

Since the return value of that function was just plain int anyway, that static_cast didn't really have any point as far as I can see ...

> lib/Target/SystemZ/SystemZISelLowering.h:248
>  preoduce typo could use a fix.

Checked in separately as  d4a7855b68d4d53f121209333d5f2796731ab1f5 <https://reviews.llvm.org/rGd4a7855b68d4d53f121209333d5f2796731ab1f5>


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69281





More information about the llvm-commits mailing list