Hi Renato,> I'm not sure about this: > > + if (!Accuracy) > + // If it's not a floating point number then it must be 'fast'. > + return getFastAccuracy(); > > Since you allow accuracies bigger than 1 in setFPAccuracy(), integers > should be treated as float. Or at least assert.the verifier checks that the accuracy operand is either a floating point number (ConstantFP) or the keyword "fast". If "Accuracy" is zero here then that means it wasn't ConstantFP. Thus it must have been the keyword "fast".> Also, I'm thinking you should carry the annotation forward on all uses > of an annotated result, or make sure the floating point library > searches recursively for annotations on any dependency of the value > being analysed.Yes, this is a possible optimization (especially useful if functions from a -ffast-math compiled module are inlined into functions from a non -ffast-math compiled module or vice versa) but it is not needed for correctness. I plan to implement optimizations using the metadata later.> About creating annotations every time, I think this could be a nice > idea for a metadata factory functionality. Something that would cache > metadata, and in case of repetition, point to the same metadata. This > could be used for other optimisations (if I recall correctly, the > debug metadata does that already).Yes, Chandler suggested it already, and I think it is a good idea.> The problem with this is that, if an optimisation pass changes one, > you must make sure the other can also be changed, or split-on-write, > and that can cause some bloated code in the optimiser, which is not > ideal.Optimizers don't (or shouldn't) change metadata because metadata is uniqued: if you change it you change it for all users. Instead new metadata has to be created. So I doubt that this is a problem in practice. Also, I think metadata is intrinsically a weak value handle, so if someone changes the metadata underneath the builder then its copy will become null. When it sees that the cached metadata is null then it can create it anew. So I think it should be possible to ensure that this works well.> I think, for now, it's acceptable. But should be on request basis > (aka, only present if -fmath options are explicitly specified). > > The rest of the patch looks sane, though. I like the idea of using > metadata, since the target code can easily ignore if it doesn't > support FP optimisations or IEEE strictness.This kind of metadata must only relax IEEE strictness (and never tighten it) because *metadata can always be discarded*. Discarding it must never result in wrong IR/transforms, thus metadata can only give additional permissions. Ciao, Duncan.
On 14 April 2012 20:34, Duncan Sands <baldrick at free.fr> wrote:> the verifier checks that the accuracy operand is either a floating point > number (ConstantFP) or the keyword "fast". If "Accuracy" is zero here > then that means it wasn't ConstantFP. Thus it must have been the keyword > "fast".I think it's assuming too much. If I write "foobar", it'd also work as "fast", or even worse, if I write "strict"... I'm not an expert in FP transformations, but as Dmitry said, there could be more than one "fast" transformations. Maybe that should be an enum somewhere, rather than an accuracy. Can you accurately propagate accuracy ratios across multiple instructions? Through multiple paths and PHI nodes? Not to mention that the "Accuracy" is also FP, which has its own accuracy problems... sigh...> This kind of metadata must only relax IEEE strictness (and never tighten > it) because *metadata can always be discarded*. Discarding it must never > result in wrong IR/transforms, thus metadata can only give additional > permissions.Makes sense. -- cheers, --renato http://systemcall.org/
Hi Renato,> On 14 April 2012 20:34, Duncan Sands<baldrick at free.fr> wrote: >> the verifier checks that the accuracy operand is either a floating point >> number (ConstantFP) or the keyword "fast". If "Accuracy" is zero here >> then that means it wasn't ConstantFP. Thus it must have been the keyword >> "fast". > > I think it's assuming too much. If I write "foobar", it'd also work as > "fast", or even worse, if I write "strict"...if you use "foobar" the verifier will reject your IR as invalid. That said, I'm not in love with the word "fast" here. Maybe "finite" would be better.> I'm not an expert in FP transformations, but as Dmitry said, there > could be more than one "fast" transformations.There's a difference between transformations that interact properly with NaNs and infinities (eg: x + 0.0 -> x) and those that don't (eg: x * 0.0 -> x). There is also a difference between those that introduce a uniformly bounded (in the inputs) relative error, and those for which the relative error introduced can be arbitrarily large depending on the inputs. I have in mind the uniformly bounded relative error ones, preserving NaNs and infinities. I guess I should say that explicitly in the LangRef changes. Maybe that should be an> enum somewhere, rather than an accuracy.I'd rather introduce additional operands in the fpmath metadata.> Can you accurately propagate accuracy ratios across multiple > instructions? Through multiple paths and PHI nodes? Not to mention > that the "Accuracy" is also FP, which has its own accuracy problems... > sigh...I don't understand the question. The metadata applies to one instruction, the accuracy loss is per instruction. A transform that introduces a relative error of 2.5 ULPs per instruction can of course result in a huge accuracy loss after a huge number of instructions. Ciao, Duncan.