[llvm-dev] Condition code in DAGCombiner::visitFADDForFMACombine?
Ryan Taylor via llvm-dev
llvm-dev at lists.llvm.org
Fri Aug 24 04:51:14 PDT 2018
I don't think propagating the NoContraction to users is the right thing to
do, as you said, it's doesn't really model the spec/intention of
NoContraction. The fadd could still possibly be contracted with another
operation.
-Ryan
On Fri, Aug 24, 2018 at 5:50 AM Nicolai Hähnle <nhaehnle at gmail.com> wrote:
> On 23.08.2018 23:11, Michael Berg wrote:
> > Can you do without the use of -fp-contract=fast (Options.AllowFPOpFusion
> == FPOpFusion::Fast ) and without Unsafe?
> > As I SPIR-V’s usage of NoContraction flies in the face of both.
>
> Yes, of course.
>
>
> > If so, you should be able to get what you want, as then you are down to
> just IR flags. You will need a model to generate the correct behavior
> though in your SPIR-V implementation wrt IR flag emissions.
>
> That's the problem though, isn't it?
>
> Of course we could start out by saying that NoContraction on an fmul
> "poisons" its users, i.e. if the value is used by an fadd, then the fadd
> doesn't get `contract` either.
>
> The problem is that this doesn't really model what the SPIR-V says, and
> so it's likely to be unreliable in the face of general transformations.
>
> I hope you appreciate the difficulty of the situation, because I have to
> admit that Sanjay's argument has some merit -- but unfortunately, the
> one spec here that's actually explicit about what should happen (and
> that we happen to have to implement) also happens to disagree with the
> LLVM LangRef.
>
> Cheers,
> Nicolai
>
>
> >
> > Regards,
> > Michael
> >
> >
> >> On Aug 23, 2018, at 1:35 PM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
> >>
> >> I think what Michael is saying that:
> >>
> >> 1. Don't set the global flag.
> >> 2. Set `contract` and `reassoc` on *all* floating point instructions by
> default.
> >> 3. Don't set those flags on instructions with NoContraction in SPIR-V.
> >>
> >> I agree with the general thrust of this approach, except for the
> significant problem that it currently doesn't work.
> >>
> >> If we follow 1-3 above, then SPIR-V fmul+fadd with NoContraction set on
> the fmul will be translated into LLVM IR fmul+fadd with contract + reassoc
> set on the fadd (but *not* on the fmul).
> >>
> >> Unfortunately, LLVM will contract those to an fma, where SPIR-V very
> explicitly says that that's forbidden.
> >>
> >> Sanjay & Michael, preferably we'd just align LLVM IR semantics here
> with SPIR-V. LangRef is very vague about what `contract` means *exactly*,
> and it does make sense to say that if you want to contract N instructions
> into a single instruction, then *all* those instructions must have
> `contract` set.
> >>
> >> If there's an argument against making this change, it'd be good if you
> could spell it out.
> >>
> >> Cheers,
> >> Nicolai
> >>
> >>
> >> On 23.08.2018 21:47, Ryan Taylor wrote:
> >>> I don't think the global fast math flag should override the
> NoContraction decoration as that's mostly the point of that decoration to
> begin with, to have fine granular control while still having a broad
> sweeping optimization. Did I miss your point? I feel like I did.
> >>> On Thu, Aug 23, 2018, 3:42 PM Michael Berg <michael_c_berg at apple.com
> <mailto:michael_c_berg at apple.com>> wrote:
> >>> Ryan,
> >>> Given that the global fast math flag overrides most fp behavior
> also
> >>> mapped to IR FMF flags as a control, I think we should remove it
> >>> from the argument. The Global fast math should override behavior
> >>> without IR flag opposition, as that is its intent under the
> >>> definition of Unsafe. Most implementations are moving away from
> the
> >>> large club of the global flag into fine granularity of control on
> >>> the IR without the Unsafe module level environment provided by
> it. This would then put the responsibility on front ends to mint the
> >>> proper expression level flags to control the desired behavior.
> >>> Regards,
> >>> Michael
> >>>> On Aug 23, 2018, at 12:13 PM, Ryan Taylor <ryta1203 at gmail.com
> >>>> <mailto:ryta1203 at gmail.com>> wrote:
> >>>>
> >>>> Michael,
> >>>>
> >>>> From the spec with regards to reassoc:
> >>>>
> >>>> – 15225 Include no re-association as a constraint required by
> >>>> the NoContraction Decoration.
> >>>>
> >>>> I don't see a solution given the situation where -fp-contract=fast
> >>>> and we want to contract. Furthermore, I think a 'nocontract' flag
> >>>> will allow the IR to be more readable in it's intention. The
> >>>> problem is you can have 2 fp arith instructions with no contracts
> >>>> and no reassoc with global fast math flag set, how can you
> >>>> differentiate between the two instructions when you want one to be
> >>>> contract and the other to be no contract?
> >>>>
> >>>> -Ryan
> >>>>
> >>>>
> >>>>
> >>>> On Thu, Aug 23, 2018 at 2:27 PM Michael Berg
> >>>> <michael_c_berg at apple.com <mailto:michael_c_berg at apple.com>>
> wrote:
> >>>>
> >>>> Ryan:
> >>>>
> >>>> I think this will take more, here is the SPIR-V relevant text
> >>>> ( https://www.khronos.org/registry/spir-v/specs/1.0/SPIRV.pdf
> ):
> >>>>
> >>>> <start>
> >>>>
> >>>> NoContraction
> >>>>
> >>>> Apply to an arithmetic instruction to indicate the operation
> >>>> cannot be combined with another instruction to form a single
> >>>> operation. For example, if applied to an OpFMul, that multiply
> >>>> can’t be combined with an addition to yield a fused
> >>>> multiply-add operation. Furthermore, such operations are not
> >>>> allowed to reassociate; e.g., add(a + add(b+c)) cannot be
> >>>> transformed to add(add(a+b) + c).
> >>>>
> >>>> <end>
> >>>>
> >>>> The problem is that the spec does not mention the reassoc
> >>>> attribute, where an implication is made to tie both together
> >>>> as one control without specifying precisely that. I do not
> >>>> think we need to add another flag. I think we can work this
> >>>> within the definition. The text implies NoContraction is both
> >>>> contract=off and reassoc=off, baring global context. Since we
> >>>> are testing for both in isContractable in some fmul contexts
> >>>> and always in fadd context, it should suffice. This and the
> >>>> global context should be manageable in a SPIR env. The above
> >>>> text does not specify if the add should control fusing or not,
> >>>> but leaves it open to interpretation.
> >>>>
> >>>> Our current definition of contract is:
> >>>>
> >>>> ``contract``
> >>>> Allow floating-point contraction (e.g. fusing a multiply
> >>>> followed by an addition into a fused multiply-and-add).
> >>>>
> >>>> Is somewhat loose, but then we do have a good deal of internal
> >>>> context no made evident that exists in the source.
> >>>> Lets put a stronger bound on the problem to solidify the
> >>>> goal. I believe front ends can be SPIR-V compliant and use
> >>>> the current framework. It seems more a management of the
> >>>> environment and IR than adding new feature controls.
> >>>>
> >>>> Regards,
> >>>> Michael
> >>>>
> >>>>
> >>>>> On Aug 23, 2018, at 10:51 AM, Ryan Taylor <
> ryta1203 at gmail.com
> >>>>> <mailto:ryta1203 at gmail.com>> wrote:
> >>>>>
> >>>>> Maybe there is a cleaner solution but it seems like adding a
> >>>>> 'nocontract' flag is close to the intention of spir-v and is
> >>>>> an easy check in the DAGCombiner without breaking anything
> >>>>> else and its intentions are very clear.
> >>>>>
> >>>>> Right now the DAGCombiner logic doesn't seem to be able to
> >>>>> handle the case of having fast math globally with instruction
> >>>>> level flags to turn off fast math. Right now, either fast
> >>>>> math is global and it's assumed everything can be contracted
> >>>>> or fast math is not global and we contract if the contract
> >>>>> flag is present on the current instruction. I could be
> >>>>> missing something.
> >>>>>
> >>>>> -Ryan
> >>>>>
> >>>>> On Thu, Aug 23, 2018 at 1:36 PM Sanjay Patel
> >>>>> <spatel at rotateright.com <mailto:spatel at rotateright.com>>
> wrote:
> >>>>>
> >>>>> If we have this:
> >>>>> r = (X * Y) + Z
> >>>>>
> >>>>> And we want that to become an fma op/node, 'contract' is
> >>>>> checked on the fadd because it is the fadd that's being
> >>>>> altered to form the (possibly non-standard) result. As I
> >>>>> think was noted earlier, whether 'contract' is set on the
> >>>>> fmul is irrelevant in our current implementation. This
> >>>>> allows the scenario where a strict fmul was inlined into
> >>>>> code with looser FP semantics, and we're still free to
> >>>>> create an fma. If the end value allows non-standard
> >>>>> behavior, then we assume that intermediate ops leading up
> >>>>> to that end value can use non-standard behavior too.
> >>>>> (cc'ing Michael Berg who did a lot of the DAG FMF work
> >>>>> recently)
> >>>>>
> >>>>> I'm not familiar with SPIR-V, but it sounds like it has
> >>>>> an inverse flag system to what we have in IR and DAG -
> >>>>> ops are presumed contract-able unless specified with
> >>>>> 'no-contract'? Not sure how to resolve that.
> >>>>>
> >>>>> If we want to change the LLVM FMF semantics, then there
> >>>>> will be breakage in the IR optimizer too (at least for
> >>>>> 'reassoc'; not sure about 'contract'). Either way, I
> >>>>> agree that we should try to clarify the LangRef about
> >>>>> this because you can't tell how things are supposed to
> >>>>> work from the current description.
> >>>>>
> >>>>>
> >>>>> On Wed, Aug 22, 2018 at 9:41 AM, Nicolai Hähnle via
> >>>>> llvm-dev <llvm-dev at lists.llvm.org
> >>>>> <mailto:llvm-dev at lists.llvm.org>> wrote:
> >>>>>
> >>>>> On 22.08.2018 13:29, Ryan Taylor wrote:
> >>>>>
> >>>>> The example starts as SPIR-V with the
> >>>>> NoContraction decoration flag on the fmul.
> >>>>>
> >>>>> I think what you are saying seems valid in that
> >>>>> if the user had put the flag on the fadd instead
> >>>>> of the fmul it would not contract and so in this
> >>>>> example the user needs to put the NoContraction
> >>>>> on the fadd though I'm not sure that's a good
> >>>>> expectation of the user. On the surface, I think
> >>>>> that if an operation didn't have the contract
> >>>>> flag than it wouldn't be contracted, regardless
> >>>>> of what flags any other operation has.
> >>>>>
> >>>>>
> >>>>> Okay, I see that the SPIR-V spec specifically calls
> >>>>> out this example.
> >>>>>
> >>>>> Unless there are conflicting requirements with
> >>>>> another frontend, I'd say we should make sure LLVM is
> >>>>> aligned with SPIR-V here. Something along the lines
> >>>>> of (in LangRef):
> >>>>>
> >>>>> ``contract``
> >>>>> Allow floating-point contraction (e.g. fusing a
> >>>>> multiply followed by
> >>>>> an addition into a fused multiply-and-add). This
> >>>>> flag must be present
> >>>>> on all affected instruction.
> >>>>>
> >>>>> And we should probably say the same about ``reassoc``
> >>>>> as well.
> >>>>>
> >>>>> Cheers,
> >>>>> Nicolai
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> On Wed, Aug 22, 2018 at 3:55 AM Nicolai Hähnle
> >>>>> via llvm-dev <llvm-dev at lists.llvm.org
> >>>>> <mailto:llvm-dev at lists.llvm.org>
> >>>>> <mailto:llvm-dev at lists.llvm.org
> >>>>> <mailto:llvm-dev at lists.llvm.org>>> wrote:
> >>>>>
> >>>>> On 21.08.2018 16:08, Ryan Taylor via llvm-dev
> >>>>> wrote:
> >>>>> > So I have a test case where:
> >>>>> >
> >>>>> > %20 = fmul nnan arcp float %15, %19
> >>>>> > %21 = fadd reassoc nnan arcp contract
> >>>>> float %20, -1.000000e+00
> >>>>> >
> >>>>> > is being contracted in DAG to fmad. Is
> >>>>> this correct since the
> >>>>> fmul has
> >>>>> > no reassoc or contract fast math flag?
> >>>>>
> >>>>> By having the reassoc and contract flags on
> >>>>> fadd, the frontend is
> >>>>> essentially saying "different rounding on the
> >>>>> value produced by the
> >>>>> fadd
> >>>>> is okay".
> >>>>>
> >>>>> So I'd say contracting this to fma is
> correct.
> >>>>>
> >>>>> Where does this code come from, and why do
> >>>>> you think contracting to fma
> >>>>> is wrong?
> >>>>>
> >>>>> Cheers,
> >>>>> Nicolai
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> >
> >>>>> > Thanks.
> >>>>> >
> >>>>> > On Mon, Aug 20, 2018 at 12:56 PM Ryan
> >>>>> Taylor <ryta1203 at gmail.com
> >>>>> <mailto:ryta1203 at gmail.com>
> >>>>> <mailto:ryta1203 at gmail.com
> >>>>> <mailto:ryta1203 at gmail.com>>
> >>>>> > <mailto:ryta1203 at gmail.com
> >>>>> <mailto:ryta1203 at gmail.com>
> >>>>> <mailto:ryta1203 at gmail.com
> >>>>> <mailto:ryta1203 at gmail.com>>>> wrote:
> >>>>> >
> >>>>> > I'm curious why the condition to fuse
> >>>>> is this:
> >>>>> >
> >>>>> > // Floating-point multiply-add with
> >>>>> intermediate rounding.
> >>>>> > bool HasFMAD = (LegalOperations &&
> >>>>> > TLI.isOperationLegal(ISD::FMAD, VT));
> >>>>> >
> >>>>> > static bool isContractable(SDNode *N)
> {
> >>>>> > SDNodeFlags F = N->getFlags();
> >>>>> > return F.hasAllowContract() ||
> >>>>> F.hasAllowReassociation();
> >>>>> > }
> >>>>> >
> >>>>> > bool CanFuse = Options.UnsafeFPMath ||
> >>>>> isContractable(N);
> >>>>> > bool AllowFusionGlobally =
> >>>>> (Options.AllowFPOpFusion ==
> >>>>> > FPOpFusion::Fast || CanFuse ||
> HasFMAD);
> >>>>> > // If the addition is not
> >>>>> contractable, do not combine.
> >>>>> > if (!AllowFusionGlobally &&
> >>>>> !isContractable(N))
> >>>>> > return SDValue();
> >>>>> >
> >>>>> > Specifically the AllowFusionGlobally,
> >>>>> I would have expected
> >>>>> > something more like:
> >>>>> >
> >>>>> > bool AllowFusionGlobally =
> >>>>> (Options.AllowFPOpFusion ==
> >>>>> > FPOpFusion::Fast && CanFuse &&
> HasFMAD);
> >>>>> >
> >>>>> > or at the very least:
> >>>>> >
> >>>>> > bool AllowFusionGlobally =
> >>>>> ((Options.AllowFPOpFusion ==
> >>>>> > FPOpFusion::Fast || CanFuse) &&
> HasFMAD);
> >>>>> >
> >>>>> > It seems that as long as the target
> >>>>> can do fmad it does do fmad
> >>>>> > since HasFMAD is true.
> >>>>> >
> >>>>> > Thanks.
> >>>>> >
> >>>>> >
> >>>>> >
> >>>>> >
> >>>>> >
> >>>>> >
> >>>>> _______________________________________________
> >>>>> > LLVM Developers mailing list
> >>>>> > llvm-dev at lists.llvm.org
> >>>>> <mailto:llvm-dev at lists.llvm.org>
> >>>>> <mailto:llvm-dev at lists.llvm.org
> >>>>> <mailto:llvm-dev at lists.llvm.org>>
> >>>>> >
> >>>>>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >>>>> >
> >>>>>
> >>>>>
> >>>>> -- Lerne, wie die Welt wirklich ist,
> >>>>> Aber vergiss niemals, wie sie sein sollte.
> >>>>>
> _______________________________________________
> >>>>> LLVM Developers mailing list
> >>>>> llvm-dev at lists.llvm.org
> >>>>> <mailto:llvm-dev at lists.llvm.org>
> >>>>> <mailto:llvm-dev at lists.llvm.org
> >>>>> <mailto:llvm-dev at lists.llvm.org>>
> >>>>>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >>>>>
> >>>>>
> >>>>>
> >>>>> -- Lerne, wie die Welt wirklich ist,
> >>>>> Aber vergiss niemals, wie sie sein sollte.
> >>>>> _______________________________________________
> >>>>> LLVM Developers mailing list
> >>>>> llvm-dev at lists.llvm.org <mailto:
> llvm-dev at lists.llvm.org>
> >>>>>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >>>>>
> >>>>>
> >>>>
> >>
> >>
> >> --
> >> Lerne, wie die Welt wirklich ist,
> >> Aber vergiss niemals, wie sie sein sollte.
> >
>
>
> --
> Lerne, wie die Welt wirklich ist,
> Aber vergiss niemals, wie sie sein sollte.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180824/8675f117/attachment.html>
More information about the llvm-dev
mailing list