This is an archive of the discontinued LLVM Phabricator instance.

[Fixed Point Arithmetic] FixedPointCast
ClosedPublic

Authored by leonardchan on Aug 12 2018, 2:48 PM.

Details

Summary

This patch is a part of https://reviews.llvm.org/D48456 in an attempt to split them up. This contains the code for casting between fixed point types and other fixed point types.

The method for converting between fixed point types is based off the convert() method in APFixedPoint.

Diff Detail

Repository
rL LLVM

Event Timeline

rjmccall added inline comments.Aug 13 2018, 12:01 PM
lib/CodeGen/CGExprScalar.cpp
1016 ↗(On Diff #160274)

In IR, this isn't really "allocating" space.

1034 ↗(On Diff #160274)

Why is this condition based on the formal types exactly matching rather than something about the FP semantics being different? Formal types can correspond to the same format, right?

We need to check for saturation if we're either (1) decreasing the magnitude of the highest usable bit or (2) going signed->unsigned, (2) we're going signed->unsigned, or (3) we're going unsigned->signed without increasing the number of integral bits. And I'd expect the checks we have to do in each case to be different.

lib/Sema/SemaExpr.cpp
5889 ↗(On Diff #160274)

Is there something I'm missing that actually diagnoses the unimplemented cases here? There's a lot of code that seems to assume that any two arithmetic types can be converted to each other, and we do prefer not to crash the compiler, especially on valid code.

leonardchan marked 3 inline comments as done.
  • Added check for if we should check for saturation when converting to a saturated fixed point type.
  • Replaced llvm_unreachable()s with temporary diagnostic to be eventually replaced when the conversions get implemented.
lib/CodeGen/CGExprScalar.cpp
1016 ↗(On Diff #160274)

My bad. Poor comment wording.

1034 ↗(On Diff #160274)

For simplicity, I more or less copied the logic from APFixedPoint::convert() which performs a saturation check that covers all of these cases if the destination semantics were saturated.

Added another condition that checks if we need to perform saturation checks. I think your (1) and (3) might be the same thing since I think we only really need to check if the magnitude decreases or if going from signed -> unsigned.

I think though that the IR emission would be the same since both cases will require checking for a change in the magnitude (via the mask). The only difference is that if going from signed->unsigned, the min saturation is zero if the value is negative.

lib/Sema/SemaExpr.cpp
5889 ↗(On Diff #160274)

The plan was to implement these in other patches. I wasn't sure if llvm_unreachable() was ok to use as a placeholder for features that will eventually be implemented.

Added diagnostics here for now to be eventually removed once these casts are implemented in later patches.

leonardchan removed a subscriber: rjmccall.
rjmccall added inline comments.Aug 15 2018, 1:13 PM
lib/CodeGen/CGExprScalar.cpp
1042 ↗(On Diff #160884)

You can just pass 0 here and it'll be zero-extended to the necessary width.

1044 ↗(On Diff #160884)

I'm sorry, but this code is really impenetrable. The variable names are non-descriptive, and there are a lot of uncommented dependencies between values, like how IsNegative propagates out, and like how it's checking without explanation that there's not a magnitude change using whether the mask ends up being all-zero. Please just assign the two components of ShouldCheckSaturation to reasonably-named local variables and then use those to guide the code-generation here.

Also, the code being generated here is pretty weird. I'm not sure the mask is helping; it might both produce better code and be easier to understand if you just broke it down into cases, like this:

if a magnitude check is required {
  auto Max = maximum value of dest type;
  auto TooHigh = IsSigned ? Builder.CreateICmpSGT(Result, Max) : Builder.CreateICmpUGT(Result, Max);
  Result = Builder.CreateSelect(TooHigh, Max, Result);
}

if signed -> unsigned {
  auto Zero = zero value of dest type;
  Result = Builder.CreateSelect(Builder.CreateICmpSLT(Result, Zero), Zero, Result);
} else if (IsSigned) {
  auto Min = minimum value of dest type;
  Result = Builder.CreateSelect(Builder.CreateICmpSLT(Result, Min), Min, Result);
}
1034 ↗(On Diff #160274)

Wow, sorry for the edit failure in that review comment. You're right, it should've been just (1) and the first (2).

Are there no fixed-point formats for which the range doesn't go up to (almost) 1? I guess there probably aren't.

lib/Sema/SemaExpr.cpp
5889 ↗(On Diff #160274)

You can still use llvm_unreachable for the cases here that aren't arithmetic types, namely all the pointer types.

leonardchan marked 7 inline comments as done.
  • Reworked logic for saturation
lib/CodeGen/CGExprScalar.cpp
1044 ↗(On Diff #160884)

My bad. I guess it seemed intuitive at first but can see how it's difficult to read.

This is the full logic and reasoning for the mask: https://reviews.llvm.org/D48661?id=153426#inline-427647

But to summarize, it's essentially for checking if the bits above the MSB changed which would indicate saturation needs to occur.

  • Mask is for getting the bits above the integral bits in the resulting type (for signed types, this also captures the sign bit)
  • Masked is the bits above the highest integral bit in the resulting type
  • Masked == Mask indicates that all the bits above the highest integral bit were ones (the value is negative) and none of them changed (no change in magnitude)
  • Mask == 0 indicates that all the bits above the highest integral bit were zeros (the value is non-negative) and none of them changed (no change in magnitude)
  • Masked == Mask || Mask == 0 therefore indicates no change in magnitude
  • !(Masked == Mask || Mask == 0) indicates a change in magnitude and we should saturate (but to save an extra instruction, this was emitted as Masked != Mask && Mask != 0
  • If we saturate, Mask also happens to be the min value we saturate to for signed types and ~Mask is also the max value we saturate to.

The reason for the IsNegative laid out like that is to prevent from emitting an extra instruction for checking a negative value.

1034 ↗(On Diff #160274)

No problem. The smallest range that a fixed point type can cover is the _Fract type which covers [-1, 1).

I think I've mentioned this before, but I would like to discuss the possibility of adding a target hook(s) for some of these operations (conversions, arithmetic when it comes). Precisely matching the emitted saturation operation is virtually impossible for a target.

lib/CodeGen/CGExprScalar.cpp
331 ↗(On Diff #160964)

What's the plan for the other conversions (int<->fix, float<->fix)? Functions for those as well?

What about EmitScalarConversion? If it cannot handle conversions of fixed-point values it should probably be made to assert, since it will likely mess up.

1017 ↗(On Diff #160964)

It is definitely simpler to always do the 'upscale+resize/downscale, [saturate], resize' in the constant evaluation case, but when emitting IR it is not as efficient. There's no need to keep the upshifted bits if you aren't interested in saturation, so in the non-saturating case it is better to keep everything in the native type sizes with '[downscale], resize, [upscale]'. This is why there are a bunch of 'sext i32 to i33' in the test cases.

Perhaps something like

if !dst.saturating
  downscale if needed
  resize
  upscale if needed
  return 

old code here, minus the 'DstFPSema.isSaturated()' which is implied

It gives a bit of duplication (or at least similar code) but I think it's an important disambiguation to make.

1052 ↗(On Diff #160964)

ConstantInt::getNullValue?

lib/Sema/Sema.cpp
537 ↗(On Diff #160964)

Will we want to support this?

leonardchan marked 6 inline comments as done.
  • Added separate case for conversions to a non-saturated type
lib/CodeGen/CGExprScalar.cpp
331 ↗(On Diff #160964)

Ideally, my plan was to have separate functions for each cast since it seems the logic for each of them is unique, other than saturation which may be abstracted to its own function and be used by the others.

I wasn't sure if I should also place the logic for these casts in EmitScalarConversion since EmitScalarConversion seemed pretty large enough and thought it was easier if we instead had separate, self contained functions for each fixed point conversion. But I'm open for hearing ideas on different ways on implementing them.

Will add the assertion.

1052 ↗(On Diff #160964)

I did not know about this method. Thanks!

lib/Sema/Sema.cpp
537 ↗(On Diff #160964)

I imagine we would want _Bool conversions since logical negation works on fixed point types and more people would probably assume by default for it to also implicitly be converted to a boolean as opposed to not.

I also don't think implementing in a later patch this will be too hard.

I think I've mentioned this before, but I would like to discuss the possibility of adding a target hook(s) for some of these operations (conversions, arithmetic when it comes). Precisely matching the emitted saturation operation is virtually impossible for a target.

Sorry I forgot to address this also. Just to make sure I understand this correctly since I haven't used these before: target hooks are essentially for emitting target specific code for some operations right? Does this mean that the EmitFixedPointConversion function should be moved to a virtual method under TargetCodeGenInfo that can be overridden and this is what get's called instead during conversion?

I think I've mentioned this before, but I would like to discuss the possibility of adding a target hook(s) for some of these operations (conversions, arithmetic when it comes). Precisely matching the emitted saturation operation is virtually impossible for a target.

Sorry I forgot to address this also. Just to make sure I understand this correctly since I haven't used these before: target hooks are essentially for emitting target specific code for some operations right? Does this mean that the EmitFixedPointConversion function should be moved to a virtual method under TargetCodeGenInfo that can be overridden and this is what get's called instead during conversion?

If this is more than just a hobby — like if there's a backend that wants to do serious work with matching fixed-point operations to hardware support — we should just add real LLVM IR support for fixed-point types instead of adding a bunch of frontend customization hooks. I don't know what better opportunity we're expecting might come along to justify that, and I don't think it's some incredibly onerous task to add a new leaf to the LLVM type hierarchy. Honestly, LLVM programmers across the board have become far too accustomed to pushing things opaquely through an uncooperative IR with an obscure muddle of intrinsics.

In the meantime, we can continue working on this code. Even if there's eventually real IR support for fixed-point types, this code will still be useful; it'll just become the core of some legalization pass in the generic backend.

Sorry I forgot to address this also. Just to make sure I understand this correctly since I haven't used these before: target hooks are essentially for emitting target specific code for some operations right? Does this mean that the EmitFixedPointConversion function should be moved to a virtual method under TargetCodeGenInfo that can be overridden and this is what get's called instead during conversion?

Yes, the thought I had was to have a virtual function in TargetCodeGenInfo that would be called first thing in EmitFixedPointConversion, and if it returns non-null it uses that value instead. It's a bit unfortunate in this instance as the only thing that doesn't work for us is the saturation, but letting you configure *parts* of the emission is a bit too specific.

If this is more than just a hobby — like if there's a backend that wants to do serious work with matching fixed-point operations to hardware support — we should just add real LLVM IR support for fixed-point types instead of adding a bunch of frontend customization hooks. I don't know what better opportunity we're expecting might come along to justify that, and I don't think it's some incredibly onerous task to add a new leaf to the LLVM type hierarchy. Honestly, LLVM programmers across the board have become far too accustomed to pushing things opaquely through an uncooperative IR with an obscure muddle of intrinsics.

In the meantime, we can continue working on this code. Even if there's eventually real IR support for fixed-point types, this code will still be useful; it'll just become the core of some legalization pass in the generic backend.

It definitely is; our downstream target requires it. As far as I know there's no real use case upstream, which is why it's likely quite hard to add any extensions to LLVM proper. Our implementation is done through intrinsics rather than an extension of the type system, as fixed-point numbers are really just integers under the hood. We've always wanted to upstream our fixed-point support (or some reasonable adaptation of it) but never gotten the impression that it would be possible since there's no upstream user.

A mail I sent to the mailing list a while back has more details on our implementation, including what intrinsics we've added: http://lists.llvm.org/pipermail/cfe-dev/2018-May/058019.html We also have an LLVM pass that converts these intrinsics into pure IR, but it's likely that it won't work properly with some parts of this upstream design.

Leonard's original proposal for this work has always been Clang-centric and never planned for implementing anything on the LLVM side, so we need to make due with what we get, but we would prefer as small a diff from upstream as possible.

lib/CodeGen/CGExprScalar.cpp
331 ↗(On Diff #160964)

Yes, you have a point. Our EmitScalarConversion does all of the fixed-point stuff at once and it's a pretty big mess.

It might be a bit confusing for users if they use EmitScalarConversion, expecting it to work on fixed-point scalars as well but then it doesn't work properly.

So long as it asserts to tell them that, it should be fine but someone else might have an opinion on the expected behavior of the function.

Sorry I forgot to address this also. Just to make sure I understand this correctly since I haven't used these before: target hooks are essentially for emitting target specific code for some operations right? Does this mean that the EmitFixedPointConversion function should be moved to a virtual method under TargetCodeGenInfo that can be overridden and this is what get's called instead during conversion?

Yes, the thought I had was to have a virtual function in TargetCodeGenInfo that would be called first thing in EmitFixedPointConversion, and if it returns non-null it uses that value instead. It's a bit unfortunate in this instance as the only thing that doesn't work for us is the saturation, but letting you configure *parts* of the emission is a bit too specific.

If this is more than just a hobby — like if there's a backend that wants to do serious work with matching fixed-point operations to hardware support — we should just add real LLVM IR support for fixed-point types instead of adding a bunch of frontend customization hooks. I don't know what better opportunity we're expecting might come along to justify that, and I don't think it's some incredibly onerous task to add a new leaf to the LLVM type hierarchy. Honestly, LLVM programmers across the board have become far too accustomed to pushing things opaquely through an uncooperative IR with an obscure muddle of intrinsics.

In the meantime, we can continue working on this code. Even if there's eventually real IR support for fixed-point types, this code will still be useful; it'll just become the core of some legalization pass in the generic backend.

It definitely is; our downstream target requires it. As far as I know there's no real use case upstream, which is why it's likely quite hard to add any extensions to LLVM proper. Our implementation is done through intrinsics rather than an extension of the type system, as fixed-point numbers are really just integers under the hood. We've always wanted to upstream our fixed-point support (or some reasonable adaptation of it) but never gotten the impression that it would be possible since there's no upstream user.

A mail I sent to the mailing list a while back has more details on our implementation, including what intrinsics we've added: http://lists.llvm.org/pipermail/cfe-dev/2018-May/058019.html We also have an LLVM pass that converts these intrinsics into pure IR, but it's likely that it won't work properly with some parts of this upstream design.

Leonard's original proposal for this work has always been Clang-centric and never planned for implementing anything on the LLVM side, so we need to make due with what we get, but we would prefer as small a diff from upstream as possible.

Has anyone actually asked LLVM whether they would accept fixed-point types into IR? I'm just a frontend guy, but it seems to me that there are advantages to directly representing these operations in a portable way even if there are no in-tree targets providing special support for them. And there are certainly in-tree targets that could provide such support if someone was motivated to do it.

Has anyone actually asked LLVM whether they would accept fixed-point types into IR? I'm just a frontend guy, but it seems to me that there are advantages to directly representing these operations in a portable way even if there are no in-tree targets providing special support for them. And there are certainly in-tree targets that could provide such support if someone was motivated to do it.

Even just adding one new LLVM IR instruction (well, intrinsic too, ideally) already 'requires' you to to
then go around and make sure it is properly handled wrt all the other instructions, optimizations, codegen.

Adding a whole new type, i suspect, would be *much* more impactful.
And since it can already be represented via existing operations on existing integer type,
it isn't obvious why that would be the right way forward.

leonardchan marked 4 inline comments as done.

Sorry I forgot to address this also. Just to make sure I understand this correctly since I haven't used these before: target hooks are essentially for emitting target specific code for some operations right? Does this mean that the EmitFixedPointConversion function should be moved to a virtual method under TargetCodeGenInfo that can be overridden and this is what get's called instead during conversion?

Yes, the thought I had was to have a virtual function in TargetCodeGenInfo that would be called first thing in EmitFixedPointConversion, and if it returns non-null it uses that value instead. It's a bit unfortunate in this instance as the only thing that doesn't work for us is the saturation, but letting you configure *parts* of the emission is a bit too specific.

Would it be simpler instead just to have the logic contained in the virtual function for TargetCodeGenInfo as opposed to returning nullptr since any custom target will end up overriding it anyway and ideally not return nullptr?

Sorry I forgot to address this also. Just to make sure I understand this correctly since I haven't used these before: target hooks are essentially for emitting target specific code for some operations right? Does this mean that the EmitFixedPointConversion function should be moved to a virtual method under TargetCodeGenInfo that can be overridden and this is what get's called instead during conversion?

Yes, the thought I had was to have a virtual function in TargetCodeGenInfo that would be called first thing in EmitFixedPointConversion, and if it returns non-null it uses that value instead. It's a bit unfortunate in this instance as the only thing that doesn't work for us is the saturation, but letting you configure *parts* of the emission is a bit too specific.

If this is more than just a hobby — like if there's a backend that wants to do serious work with matching fixed-point operations to hardware support — we should just add real LLVM IR support for fixed-point types instead of adding a bunch of frontend customization hooks. I don't know what better opportunity we're expecting might come along to justify that, and I don't think it's some incredibly onerous task to add a new leaf to the LLVM type hierarchy. Honestly, LLVM programmers across the board have become far too accustomed to pushing things opaquely through an uncooperative IR with an obscure muddle of intrinsics.

In the meantime, we can continue working on this code. Even if there's eventually real IR support for fixed-point types, this code will still be useful; it'll just become the core of some legalization pass in the generic backend.

It definitely is; our downstream target requires it. As far as I know there's no real use case upstream, which is why it's likely quite hard to add any extensions to LLVM proper. Our implementation is done through intrinsics rather than an extension of the type system, as fixed-point numbers are really just integers under the hood. We've always wanted to upstream our fixed-point support (or some reasonable adaptation of it) but never gotten the impression that it would be possible since there's no upstream user.

A mail I sent to the mailing list a while back has more details on our implementation, including what intrinsics we've added: http://lists.llvm.org/pipermail/cfe-dev/2018-May/058019.html We also have an LLVM pass that converts these intrinsics into pure IR, but it's likely that it won't work properly with some parts of this upstream design.

Leonard's original proposal for this work has always been Clang-centric and never planned for implementing anything on the LLVM side, so we need to make due with what we get, but we would prefer as small a diff from upstream as possible.

Has anyone actually asked LLVM whether they would accept fixed-point types into IR? I'm just a frontend guy, but it seems to me that there are advantages to directly representing these operations in a portable way even if there are no in-tree targets providing special support for them. And there are certainly in-tree targets that could provide such support if someone was motivated to do it.

I haven't brought this up to llvm-dev, but the main reason for why we decided to only implement this in clang was because we thought it would be a lot harder pushing a new type into upstream llvm, especially since a lot of the features can be supported on the frontend using clang's existing infrastructure. We are open to adding fixed point types to LLVM IR in the future though once all the frontend stuff is laid out and if the community is willing to accept it.

Has anyone actually asked LLVM whether they would accept fixed-point types into IR? I'm just a frontend guy, but it seems to me that there are advantages to directly representing these operations in a portable way even if there are no in-tree targets providing special support for them. And there are certainly in-tree targets that could provide such support if someone was motivated to do it.

I haven't brought this up to llvm-dev, but the main reason for why we decided to only implement this in clang was because we thought it would be a lot harder pushing a new type into upstream llvm, especially since a lot of the features can be supported on the frontend using clang's existing infrastructure. We are open to adding fixed point types to LLVM IR in the future though once all the frontend stuff is laid out and if the community is willing to accept it.

Mostly I'm reluctant to add a ton of customization points to get more optimizable IR patterns from the frontend when I think it's really clear that the right thing to do is to represent these operations semantically through LLVM IR.

If LLVM people don't want to add an llvm::FixedPointType then we should at least add portable intrinsics for them instead of target intrinsics, in which case there's still no point in customizing this in the frontend.

Has anyone actually asked LLVM whether they would accept fixed-point types into IR? I'm just a frontend guy, but it seems to me that there are advantages to directly representing these operations in a portable way even if there are no in-tree targets providing special support for them. And there are certainly in-tree targets that could provide such support if someone was motivated to do it.

Even just adding one new LLVM IR instruction (well, intrinsic too, ideally) already 'requires' you to to
then go around and make sure it is properly handled wrt all the other instructions, optimizations, codegen.

Adding a whole new type, i suspect, would be *much* more impactful.
And since it can already be represented via existing operations on existing integer type,
it isn't obvious why that would be the right way forward.

Very few things in LLVM actually try to exhaustively handle all operations. There are a
couple of generic predicates on Instruction like mayHaveSideEffects, there's
serialization/.ll-writing, and there's code-gen. The first two are not hard at all
to implement, and it'd be quite simple to write a legalization pass in code-gen that
turns all these operations into integer operations and which could easily be customized
to support targets that want to do something more precise.

The advantages of having real IR support include:

  • It's significant simpler for frontends. All of this logic in Clang will need to be reimplemented in any other frontend that wants to support fixed-point types.
  • It's much easier to test. The frontend needs to handle a lot of different code patterns that ultimately turn into the same small set of basic operations, like compound arithmetic and atomic operations and a million different things that are supposed to trigger implicit conversions. It's ver frustrating to write tests for these things when constants aren't readable and the generated IR for every operation is a ten-instruction sequence.
  • It's much easier to analyze and optimize. I'm sure some fixed-point optimizations can be done generically over the underlying integer ops, but many others would be much more difficult if not impossible — e.g. I would assume that the lowering of padded unsigned values exploits an assumption that the padding bit is zero, which a generic optimization cannot know.
  • All those ten-instruction sequences add up in their compile-time impact on every stage of the pipeline.

I'm not saying it's an open-and-shut case, but LLVM is supposed to be an abstraction layer
over more than just the concerns of code-generation, and even those concerns don't
obviously point towards frontend expansion.

Like I said, if this is just a hobby and we don't really care about supporting this as a feature
beyond just checking a box, frontend expansion is definitely the easier approach for checking
that box. But if we want a high-quality implementation of fixed-point types, we should
represent them directly in LLVM IR.

Would it be simpler instead just to have the logic contained in the virtual function for TargetCodeGenInfo as opposed to returning nullptr since any custom target will end up overriding it anyway and ideally not return nullptr?

I guess that's also possible. I figured it would be clearer to have the generic logic in the more obvious location (CGExprScalar) and fall back to the custom handling if needed. Many of the handlers in TargetCodeGenInfo are empty.

I haven't brought this up to llvm-dev, but the main reason for why we decided to only implement this in clang was because we thought it would be a lot harder pushing a new type into upstream llvm, especially since a lot of the features can be supported on the frontend using clang's existing infrastructure. We are open to adding fixed point types to LLVM IR in the future though once all the frontend stuff is laid out and if the community is willing to accept it.

I know there are multiple threads on llvm-dev asking about fixed-point support (some of them from us) and the verdict usually seems to be "no new types, use integers and intrinsics". This has worked fairly well for us, so it should be possible to adapt the design for upstream if there is a desire to have it.

I do not think a new type is required. As there are no in-tree architectures with native fixed-point support (as far as I know? Does AVR or ARM support it, perhaps? Probably not enough to cover the entire E-C spec, though.), there isn't really much to warrant adding a new type (or even new IR operators) for it. Furthermore, many of the operations on fixed-point are simple integer operations (except ones like saturation/saturating operations, multiplication, division etc) so those would be better off implemented in terms of those operators rather than inventing new ones that do pretty much the same thing.

Very few things in LLVM actually try to exhaustively handle all operations. There are a
couple of generic predicates on Instruction like mayHaveSideEffects, there's
serialization/.ll-writing, and there's code-gen. The first two are not hard at all
to implement, and it'd be quite simple to write a legalization pass in code-gen that
turns all these operations into integer operations and which could easily be customized
to support targets that want to do something more precise.

I certainly support the idea of representing some (not all) fixed-point operations as intrinsics (or instructions, although I suspect that's more work than one might think) and converting them into integer instructions for targets which do not care to support the specific operation natively. A type is overkill; fixed-point values are 'bags of bits' like LLVM integers, just with a different interpretation of the bits. It's no different than the distinction between signed and unsigned, and we model that on the LLVM level through the operations rather than through the types.

Creating instruction/intrinsic definitions of fixed-point operations does come with problems. If an IR producer wants to emit fixed-point operations with slightly different semantics than what our instructions/intrinsics have, they're out of luck. It might also be a problem for targets that have fixed-point support, but cannot perform the operations in exactly the same manner as the IR operation specifies. Making the semantics of the operation too tight could cause this to happen, but making them too loose makes it hard to really do anything with it. This is another argument for frontend customization. Every operation also introduces a bit of optimization/codegen work, unlike pure integer operations which the passes already know.

Still, I think intrinsics for the complicated operations would provide a good balance.

The advantages of having real IR support include:

  • It's significant simpler for frontends. All of this logic in Clang will need to be reimplemented in any other frontend that wants to support fixed-point types.

As I mentioned, this only simplifies things if the semantics of the operations work for the other frontend/language/target.

  • It's much easier to test. The frontend needs to handle a lot of different code patterns that ultimately turn into the same small set of basic operations, like compound arithmetic and atomic operations and a million different things that are supposed to trigger implicit conversions. It's ver frustrating to write tests for these things when constants aren't readable and the generated IR for every operation is a ten-instruction sequence.

I think this is primarily a problem for saturating operations, as those require a whole bunch of conditional assignment. Nonsaturating multiplication is only about 5 instructions, but it's still useful to have an operation for it, and division is about the same (although I have some input on division that might make it a bit more complex than that).

  • It's much easier to analyze and optimize. I'm sure some fixed-point optimizations can be done generically over the underlying integer ops, but many others would be much more difficult if not impossible — e.g. I would assume that the lowering of padded unsigned values exploits an assumption that the padding bit is zero, which a generic optimization cannot know.

I think the representation that we landed on (for now) was that the padding bit is undefined rather than zero. I'm not sure I'm a fan of that representation, though.

If it was the case that it always was zero and we only use integer operations, this property should shine through, though. It would be a deficiency in the integer analysis otherwise.

  • All those ten-instruction sequences add up in their compile-time impact on every stage of the pipeline.

I'm not saying it's an open-and-shut case, but LLVM is supposed to be an abstraction layer
over more than just the concerns of code-generation, and even those concerns don't
obviously point towards frontend expansion.

Like I said, if this is just a hobby and we don't really care about supporting this as a feature
beyond just checking a box, frontend expansion is definitely the easier approach for checking
that box. But if we want a high-quality implementation of fixed-point types, we should
represent them directly in LLVM IR.

The former is definitely what has happened to the C99 _Complex feature, and it would be nice if we could avoid that for fixed-point.

Should we discuss this with someone on the LLVM side?

I made a post on llvm-dev (http://lists.llvm.org/pipermail/llvm-dev/2018-August/125433.html) to see if other people have opinions on this. In the meantime, do you think I should make a separate patch that moves this into an LLVM intrinsic function?

I made a post on llvm-dev (http://lists.llvm.org/pipermail/llvm-dev/2018-August/125433.html) to see if other people have opinions on this. In the meantime, do you think I should make a separate patch that moves this into an LLVM intrinsic function?

Yeah, I think that even if LLVM decides they don't want to include first-class IR support for fixed-point types, it makes more sense to provide standard intrinsics for these operations than to do all of the lowering in the frontend.

Accidentally noticed about this review via cfe-commits. @NoQ, the change in the ExprEngine looks a bit dangerous to me. Could you please check?

lib/StaticAnalyzer/Core/ExprEngineC.cpp
419 ↗(On Diff #161298)

Should we consider this construction as unsupported rather than supported as a normal cast?

NoQ added inline comments.Aug 22 2018, 12:32 PM
lib/StaticAnalyzer/Core/ExprEngineC.cpp
419 ↗(On Diff #161298)

Uhm, this code seems to be held together by magic. We squeeze all sorts of casts (eg., float casts) into a subroutine that deals with casts of lvalues (!?) Fortunately, it dissolves into SValBuilder::evalCast() pretty quickly, so we don't really get punished for that. So it's not this patch's fault but our technical debt. I guess this change on its own doesn't make things worse, so i'm ok with it.

@ebevhan @rjmccall Seeing that the saturation intrinsic in https://reviews.llvm.org/D52286 should be put on hold for now, would it be fine to submit this patch as is? Then if the intrinsic is finished, come back to this and update this patch to use the intrinsic?

@ebevhan @rjmccall Seeing that the saturation intrinsic in https://reviews.llvm.org/D52286 should be put on hold for now, would it be fine to submit this patch as is? Then if the intrinsic is finished, come back to this and update this patch to use the intrinsic?

Okay, but please un-target-hook it.

I agree with John, after that I think it's fine for me.

leonardchan updated this revision to Diff 169500.
  • Removed target hook
This revision is now accepted and ready to land.Oct 14 2018, 10:21 PM
ebevhan added inline comments.Oct 15 2018, 3:33 AM
lib/CodeGen/CGExprScalar.cpp
1019 ↗(On Diff #169500)

It's not in TargetCodeGenInfo any more.

leonardchan marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.