[PATCH] D90554: [CostModel] remove cost-kind predicate for intrinsics in basic TTI implementation

[Kevin Neal via llvm-commits]

From: llvm-commits <llvm-commits-bounces at lists.llvm.org> On Behalf Of Sanjay Patel via llvm-commits
Sent: Friday, November 20, 2020 3:16 PM
To: Eric Christopher <echristo at gmail.com>
Cc: Mehdi AMINI <reviews+D90554+public+a451c79bd8cad907 at reviews.llvm.org>; Fangrui Song <maskray at google.com>; nikic at php.net; llvm-commits <llvm-commits at lists.llvm.org>; dougpuob at gmail.com; sam.parker at arm.com; 88888yl at gmail.com; Kristof Beyls <kristof.beyls at arm.com>; higuoxing at gmail.com
Subject: Re: [PATCH] D90554: [CostModel] remove cost-kind predicate for intrinsics in basic TTI implementation


EXTERNAL


On Fri, Nov 20, 2020 at 2:07 PM Eric Christopher <echristo at gmail.com<mailto:echristo at gmail.com>> wrote:


On Fri, Nov 20, 2020 at 1:44 PM Mehdi AMINI via Phabricator <reviews at reviews.llvm.org<mailto:reviews at reviews.llvm.org>> wrote:
mehdi_amini added a comment.

In D90554#2408163 <https://reviews.llvm.org/D90554#2408163<https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD90554%232408163&data=04%7C01%7Ckevin.neal%40sas.com%7Ca394536816e94978aa4b08d88d912fd8%7Cb1c14d5c362545b3a4309552373a0c2f%7C0%7C0%7C637415001999363800%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=07Rq%2FkLxVxS6bLZdLE966u1nZXwspCWP3pbyu7reyiI%3D&reserved=0>>, @spatel wrote:

> So to clarify policy, we reverted patches when (1) a test based on an experimental intrinsic was crashing and (2) that crash is easily reproducible independent of this patch as shown here:
> 7ae346434 <https://reviews.llvm.org/rG7ae346434a5f51b81ebaeeb50bd5d97666ee288b<https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FrG7ae346434a5f51b81ebaeeb50bd5d97666ee288b&data=04%7C01%7Ckevin.neal%40sas.com%7Ca394536816e94978aa4b08d88d912fd8%7Cb1c14d5c362545b3a4309552373a0c2f%7C0%7C0%7C637415001999363800%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Liq%2BFs0ivTaVQ9qcjeKb0j9mRlb3Q9clzWZrCEARJkk%3D&reserved=0>>
>
> I understand that we revert first and ask questions later, but should that be the rule for experimental code?

In that case it seems like there was user-visible impact for some clang user according to @brooksmoses comment above? It isn't like an arbitrary Fuzzer was plugged to the system. IMO a revert is "low cost" and easy to re-land with a fix here, little downside to do this?

Appreciated for sure. I'm not sure how "experimental" we want to consider the rounding math intrinsics at this point. It's an interesting question and probably should be raised on llvm-dev. If it were something not user visible or really experimental I probably wouldn't have reverted, but given it's a fairly common use case for developers to use the option it seemed on the "not really as experimental as it sounds" area. I think the intrinsics themselves might be experimental in the "we could change these radically" rather than "we don't really expect these to work".

Yeah, I agree. The fixes in this case were easy because I could just about tell from the assert and backtrace where the problem was, but it might be more controversial for some theoretical future problem.
So maybe the real question is when to remove the "experimental" label. For example, we went years with the reduction intrinsics and recently promoted them because it was clear they had become part of the standard world. If the constrained intrinsics are already in mainstream use via clang, then we should treat them as 1st class IR?

Well, the experimental constrained intrinsics only work on X86, PowerPC, and SystemZ currently for starters. The metadata in the intrinsics is not set correctly by clang in some cases if you use any relevant math #pragma. There are rules that are supposed to be followed, but we aren’t enforcing them yet in LLVM because clang isn’t ready. And the inliner isn’t fixed to handle them yet.

Also, clang’s versions of isnan(), fpclassify(), and a few others will trap if given an SNaN despite IEEE 754 saying they shouldn’t. I’m not sure how to fix that, and it may mean the addition of more constrained intrinsics.

So I don’t think they should be moved out of experimental until we have a clang+llvm implementation that is much closer to complete.
--
Kevin P. Neal
SAS/C and SAS/C++ Compiler
Compute Services
SAS Institute, Inc.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201123/a0b8a3e1/attachment.html>