[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan abandoned this revision.
ebevhan added a comment.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82663

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In D82663#2153834 , @rjmccall wrote:

> I don't understand.  The problem statement as I understood it is that using 
> unsigned intrinsics to do unsigned-with-padding operations is leading to poor 
> code-gen, so you want to start using signed intrinsics, which you can safely 
> do because unsigned-with-padding types are intended to be exactly signed 
> types with a dynamic range restriction to non-negative values.  The result of 
> that operation is still logically an unsigned-with-padding value; there's no 
> need to return back a modified semantic that says that the result is really a 
> signed value because it's *not* really a signed value, you're just computing 
> it a different way.  I also don't understand why you think need a modified 
> semantics value in the first place as opposed to just using a more complex 
> condition when deciding which intrinsics to use.


Well, it's not just about intrinsics. For saturating operations, the common 
semantic width is currently narrowed by one bit to get the correct saturating 
behavior on the operation afterwards. This affects the conversion to the common 
semantic, since the type will be one bit narrower.

But I realize now that it probably doesn't matter if we simply don't do that 
narrowing when constructing the common semantic. That way it will always have 
the right width and we can just do as you say and select the right intrinsic 
based on the padding bit information. Alright, that should probably work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D82663#2153176 , @ebevhan wrote:

> In D82663#2144551 , @rjmccall wrote:
>
> > In D82663#2144219 , @ebevhan wrote:
> >
> > > In D82663#2142426 , @rjmccall 
> > > wrote:
> > >
> > > > Would it be sensible to use a technical design more like what the 
> > > > matrix folks are doing, where LLVM provides a small interface for 
> > > > emitting operations with various semantics?  FixedPointSemantics would 
> > > > move to that header, and Clang would just call into it.  That way you 
> > > > get a lot more flexibility in how you generate code, and the Clang 
> > > > IRGen logic is still transparently correct.  If you want to add 
> > > > intrinsics or otherwise change the IR patterns used for various 
> > > > operations, you don't have to rewrite a bunch of Clang IRGen logic 
> > > > every time, you just have to update the tests.  It'd then be pretty 
> > > > straightforward to have internal helper functions in that interface for 
> > > > computing things like whether you should use signed or unsigned 
> > > > intrinsics given the desired FixedPointSemantics.
> > >
> > >
> > > This seems like a reasonable thing to do for other reasons as well. Also 
> > > moving the actual APFixedPoint class to LLVM would make it easier to 
> > > reuse the fixedpoint calculation code for constant folding in LLVM, for 
> > > example.
> >
> >
> > Just to say "I told you so", I'm pretty sure I told people this would 
> > happen. :)
>
>
> Well, transferring the fixed point concept over to LLVM felt like it would 
> happen sooner or later, for the reasons we've discussed here as well as for 
> other reasons. I'm not sure that the discrepancies between the Clang and LLVM 
> semantics were predicted to be the driving factor behind the move, though.
>
> >>> My interest here is mainly in (1) keeping IRGen's logic as obviously 
> >>> correct as possible, (2) not hard-coding a bunch of things that really 
> >>> feel like workarounds for backend limitations, and (3) not complicating 
> >>> core abstractions like FixedPointSemantics with unnecessary extra rules 
> >>> for appropriate use, like having to pass an extra "for codegen" flag to 
> >>> get optimal codegen.  If IRGen can just pass down the high-level 
> >>> semantics it wants to some library that will make intelligent decisions 
> >>> about how to emit IR, that seems best.
> >> 
> >> Just to clarify something here; would the interface in LLVM still emit 
> >> signed operations for unsigned with padding?
> > 
> > If that's the best IR pattern to emit, yes.
> > 
> >> If so, why does dealing with the padding bit detail in LLVM rather than 
> >> Clang make more sense?
> > 
> > Because frontends should be able to just say "I have a value of a type with 
> > these semantics, I need you to do these operations, go do them".  The whole 
> > purpose of this interface would be to go down a level of abstraction by 
> > picking the best IR to represent those operations.
> > 
> > Maybe we're not in agreement about what this interface looks like — I'm 
> > imagining something like
> > 
> >   struct FixedPointEmitter {
> > IRBuilder 
> > FixedPointEmitter(IRBuilder ) : B(B) {}
> >   
> > Value *convert(Value *src, FixedPointSemantics srcSemantics, 
> > FixedPointSemantics destSemantics);
> > Value *add(Value *lhs, FixedPointSemantics lhsSemantics, Value *rhs, 
> > FixedPointSemantics rhsSemantics)
> >   };
>
> I've spent some time going over this and trying to figure out how it would 
> work. I think the interface seems fine on the surface, but I don't see how it 
> directly solves the issues at hand. Regardless of whether this is factored 
> out to LLVM, we still have the issue that we have to massage the semantic 
> **somewhere** in order to get different behavior for certain kinds of 
> semantics during binop codegen.
>
> Since the binop functions take two different semantics, it must perform 
> conversions internally to get the values to match up before the operation. 
> This would probably just be to the common semantic between the two, and it 
> would then return the Value in the common semantic (since we don't know what 
> to convert back to).
>
> In order for the binop functions to have special behavior for padded 
> unsigned, they would need to modify the common semantic internally in order 
> to get the conversion right. This means that the semantic of the returned 
> Value will **not** be what you would normally get from `getCommonSemantic`, 
> so the caller of the function will have no idea what the semantic of the 
> returned value is.
>
> Even if we only treat it as an internal detail of the binop functions and 
> never expose this 'modified' semantic externally, this means we might end up 
> with superfluous operations since (for padded saturating unsigned) we will be 
> 

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In D82663#2144551 , @rjmccall wrote:

> In D82663#2144219 , @ebevhan wrote:
>
> > In D82663#2142426 , @rjmccall 
> > wrote:
> >
> > > Would it be sensible to use a technical design more like what the matrix 
> > > folks are doing, where LLVM provides a small interface for emitting 
> > > operations with various semantics?  FixedPointSemantics would move to 
> > > that header, and Clang would just call into it.  That way you get a lot 
> > > more flexibility in how you generate code, and the Clang IRGen logic is 
> > > still transparently correct.  If you want to add intrinsics or otherwise 
> > > change the IR patterns used for various operations, you don't have to 
> > > rewrite a bunch of Clang IRGen logic every time, you just have to update 
> > > the tests.  It'd then be pretty straightforward to have internal helper 
> > > functions in that interface for computing things like whether you should 
> > > use signed or unsigned intrinsics given the desired FixedPointSemantics.
> >
> >
> > This seems like a reasonable thing to do for other reasons as well. Also 
> > moving the actual APFixedPoint class to LLVM would make it easier to reuse 
> > the fixedpoint calculation code for constant folding in LLVM, for example.
>
>
> Just to say "I told you so", I'm pretty sure I told people this would happen. 
> :)


Well, transferring the fixed point concept over to LLVM felt like it would 
happen sooner or later, for the reasons we've discussed here as well as for 
other reasons. I'm not sure that the discrepancies between the Clang and LLVM 
semantics were predicted to be the driving factor behind the move, though.

>>> My interest here is mainly in (1) keeping IRGen's logic as obviously 
>>> correct as possible, (2) not hard-coding a bunch of things that really feel 
>>> like workarounds for backend limitations, and (3) not complicating core 
>>> abstractions like FixedPointSemantics with unnecessary extra rules for 
>>> appropriate use, like having to pass an extra "for codegen" flag to get 
>>> optimal codegen.  If IRGen can just pass down the high-level semantics it 
>>> wants to some library that will make intelligent decisions about how to 
>>> emit IR, that seems best.
>> 
>> Just to clarify something here; would the interface in LLVM still emit 
>> signed operations for unsigned with padding?
> 
> If that's the best IR pattern to emit, yes.
> 
>> If so, why does dealing with the padding bit detail in LLVM rather than 
>> Clang make more sense?
> 
> Because frontends should be able to just say "I have a value of a type with 
> these semantics, I need you to do these operations, go do them".  The whole 
> purpose of this interface would be to go down a level of abstraction by 
> picking the best IR to represent those operations.
> 
> Maybe we're not in agreement about what this interface looks like — I'm 
> imagining something like
> 
>   struct FixedPointEmitter {
> IRBuilder 
> FixedPointEmitter(IRBuilder ) : B(B) {}
>   
> Value *convert(Value *src, FixedPointSemantics srcSemantics, 
> FixedPointSemantics destSemantics);
> Value *add(Value *lhs, FixedPointSemantics lhsSemantics, Value *rhs, 
> FixedPointSemantics rhsSemantics)
>   };

I've spent some time going over this and trying to figure out how it would 
work. I think the interface seems fine on the surface, but I don't see how it 
directly solves the issues at hand. Regardless of whether this is factored out 
to LLVM, we still have the issue that we have to massage the semantic 
**somewhere** in order to get different behavior for certain kinds of semantics 
during binop codegen.

Since the binop functions take two different semantics, it must perform 
conversions internally to get the values to match up before the operation. This 
would probably just be to the common semantic between the two, and it would 
then return the Value in the common semantic (since we don't know what to 
convert back to).

In order for the binop functions to have special behavior for padded unsigned, 
they would need to modify the common semantic internally in order to get the 
conversion right. This means that the semantic of the returned Value will 
**not** be what you would normally get from `getCommonSemantic`, so the caller 
of the function will have no idea what the semantic of the returned value is.

Even if we only treat it as an internal detail of the binop functions and never 
expose this 'modified' semantic externally, this means we might end up with 
superfluous operations since (for padded saturating unsigned) we will be forced 
to trunc the result by one bit to match the real common semantic before we 
return.

The only solution I can think of is to also return the semantic of the result 
Value, which feels like it makes the interface pretty bulky.

I can start off by moving APFixedPoint and 

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D82663#2144219 , @ebevhan wrote:

> In D82663#2142426 , @rjmccall wrote:
>
> > Would it be sensible to use a technical design more like what the matrix 
> > folks are doing, where LLVM provides a small interface for emitting 
> > operations with various semantics?  FixedPointSemantics would move to that 
> > header, and Clang would just call into it.  That way you get a lot more 
> > flexibility in how you generate code, and the Clang IRGen logic is still 
> > transparently correct.  If you want to add intrinsics or otherwise change 
> > the IR patterns used for various operations, you don't have to rewrite a 
> > bunch of Clang IRGen logic every time, you just have to update the tests.  
> > It'd then be pretty straightforward to have internal helper functions in 
> > that interface for computing things like whether you should use signed or 
> > unsigned intrinsics given the desired FixedPointSemantics.
>
>
> This seems like a reasonable thing to do for other reasons as well. Also 
> moving the actual APFixedPoint class to LLVM would make it easier to reuse 
> the fixedpoint calculation code for constant folding in LLVM, for example.


Just to say "I told you so", I'm pretty sure I told people this would happen. :)

>> My interest here is mainly in (1) keeping IRGen's logic as obviously correct 
>> as possible, (2) not hard-coding a bunch of things that really feel like 
>> workarounds for backend limitations, and (3) not complicating core 
>> abstractions like FixedPointSemantics with unnecessary extra rules for 
>> appropriate use, like having to pass an extra "for codegen" flag to get 
>> optimal codegen.  If IRGen can just pass down the high-level semantics it 
>> wants to some library that will make intelligent decisions about how to emit 
>> IR, that seems best.
> 
> Just to clarify something here; would the interface in LLVM still emit signed 
> operations for unsigned with padding?

If that's the best IR pattern to emit, yes.

> If so, why does dealing with the padding bit detail in LLVM rather than Clang 
> make more sense?

Because frontends should be able to just say "I have a value of a type with 
these semantics, I need you to do these operations, go do them".  The whole 
purpose of this interface would be to go down a level of abstraction by picking 
the best IR to represent those operations.

Maybe we're not in agreement about what this interface looks like — I'm 
imagining something like

  struct FixedPointEmitter {
IRBuilder 
FixedPointEmitter(IRBuilder ) : B(B) {}
  
Value *convert(Value *src, FixedPointSemantics srcSemantics, 
FixedPointSemantics destSemantics);
Value *add(Value *lhs, FixedPointSemantics lhsSemantics, Value *rhs, 
FixedPointSemantics rhsSemantics)
  };



> The regular IRBuilder is relatively straightforward in its behavior.  I 
> suspect that if anything, LLVM would be equally unwilling to take to take 
> IRBuilder patches that emitted signed intrinsics for certain unsigned 
> operations only due to a detail in Embedded-C's implementation of fixedpoint 
> support.

Most things in IRBuilder don't have variant representations beyond what's 
expressed by the value type.  The fact that we've chosen to do so here 
necessitates a more complex interface.

> Regarding backend limitations, I guess I could propose an alternate solution. 
> If we change FixedPointSemantics to strip the padding bit for both saturating 
> and nonsaturating operations, it may be possible to detect in isel that the 
> corresponding signed operation could be used instead when we promote the type 
> of an unsigned one. For example, if we emit i15 umul.fix scale 15, we could 
> tell in lowering that i16 smul.fix scale 15 is legal and use that instead. 
> Same for all the other intrinsics, including the non-fixedpoint 
> uadd.sat/usub.sat.
> 
> The issue with this approach (which is why I didn't really want to do it) is 
> that it's not testable. No upstream target has these intrinsics marked as 
> legal. I doubt anyone would accept a patch with no tests.
>  It may also be less efficient than just emitting the signed operations in 
> the first place, because we are forced to trunc and zext in IR before and 
> after every operation.

I don't want to tell you the best IR to use to get good code; I just want 
frontends to have a reasonably canonical interface to use that matches up well 
with the information we have.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In D82663#2142426 , @rjmccall wrote:

> Would it be sensible to use a technical design more like what the matrix 
> folks are doing, where LLVM provides a small interface for emitting 
> operations with various semantics?  FixedPointSemantics would move to that 
> header, and Clang would just call into it.  That way you get a lot more 
> flexibility in how you generate code, and the Clang IRGen logic is still 
> transparently correct.  If you want to add intrinsics or otherwise change the 
> IR patterns used for various operations, you don't have to rewrite a bunch of 
> Clang IRGen logic every time, you just have to update the tests.  It'd then 
> be pretty straightforward to have internal helper functions in that interface 
> for computing things like whether you should use signed or unsigned 
> intrinsics given the desired FixedPointSemantics.


This seems like a reasonable thing to do for other reasons as well. Also moving 
the actual APFixedPoint class to LLVM would make it easier to reuse the 
fixedpoint calculation code for constant folding in LLVM, for example.

> My interest here is mainly in (1) keeping IRGen's logic as obviously correct 
> as possible, (2) not hard-coding a bunch of things that really feel like 
> workarounds for backend limitations, and (3) not complicating core 
> abstractions like FixedPointSemantics with unnecessary extra rules for 
> appropriate use, like having to pass an extra "for codegen" flag to get 
> optimal codegen.  If IRGen can just pass down the high-level semantics it 
> wants to some library that will make intelligent decisions about how to emit 
> IR, that seems best.

Just to clarify something here; would the interface in LLVM still emit signed 
operations for unsigned with padding?  If so, why does dealing with the padding 
bit detail in LLVM rather than Clang make more sense? The regular IRBuilder is 
relatively straightforward in its behavior.  I suspect that if anything, LLVM 
would be equally unwilling to take to take IRBuilder patches that emitted 
signed intrinsics for certain unsigned operations only due to a detail in 
Embedded-C's implementation of fixedpoint support.

I could remove the special behavior from FixedPointSemantics and only deal with 
it in EmitFixedPointBinOp instead. I agree that the FixedPointSemantics 
interface is muddied by the extra parameter.
Unless I alter the semantics object it might make EmitFixedPointBinOp rather 
messy, though.

---

Regarding backend limitations, I guess I could propose an alternate solution. 
If we change FixedPointSemantics to strip the padding bit for both saturating 
and nonsaturating operations, it may be possible to detect in isel that the 
corresponding signed operation could be used instead when we promote the type 
of an unsigned one. For example, if we emit i15 umul.fix scale 15, we could 
tell in lowering that i16 smul.fix scale 15 is legal and use that instead. Same 
for all the other intrinsics, including the non-fixedpoint uadd.sat/usub.sat.

The issue with this approach (which is why I didn't really want to do it) is 
that it's not testable. No upstream target has these intrinsics marked as 
legal. I doubt anyone would accept a patch with no tests.
It may also be less efficient than just emitting the signed operations in the 
first place, because we are forced to trunc and zext in IR before and after 
every operation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Would it be sensible to use a technical design more like what the matrix folks 
are doing, where LLVM provides a small interface for emitting operations with 
various semantics?  FixedPointSemantics would move to that header, and Clang 
would just call into it.  That way you get a lot more flexibility in how you 
generate code, and the Clang IRGen logic is still transparently correct.  If 
you want to add intrinsics or otherwise change the IR patterns used for various 
operations, you don't have to rewrite a bunch of Clang IRGen logic every time, 
you just have to update the tests.  It'd then be pretty straightforward to have 
internal helper functions in that interface for computing things like whether 
you should use signed or unsigned intrinsics given the desired 
FixedPointSemantics.

My interest here is mainly in (1) keeping IRGen's logic as obviously correct as 
possible, (2) not hard-coding a bunch of things that really feel like 
workarounds for backend limitations, and (3) not complicating core abstractions 
like FixedPointSemantics with unnecessary extra rules for appropriate use, like 
having to pass an extra "for codegen" flag to get optimal codegen.  If IRGen 
can just pass down the high-level semantics it wants to some library that will 
make intelligent decisions about how to emit IR, that seems best.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

> It wouldn't just be restricted to fixed-point intrinsics, though. It would 
> have to be added to intrinsics like uadd.sat and usub.sat as well, which 
> aren't really tied to fixed-point at all.

Oh wait, sorry. I think I'm starting to understand now. You're saying that if 
you're using the padding bit in the first place, ISel shouldn't need to perform 
the underlying shift during integral promotions, but we do it anyway still. 
Yeah it seems a lot of this could be addressed simply by just using the 
corresponding signed intrinsics.

I guess I'd be ok with purely making this a clang change for now, but if other 
frontends see interest in the unsigned padding bit then we could migrate this 
to LLVM down the line.




Comment at: clang/lib/Basic/FixedPoint.cpp:143-158
+  // For codegen purposes, make unsigned with padding semantics signed instead.
+  // This means that we will generate signed operations. The result from these
+  // operations is defined, since ending up with a negative result is undefined
+  // for nonsaturating semantics, and for saturating semantics we will
+  // perform a clamp-to-zero in the last conversion to result semantics (since
+  // we are going from saturating signed to saturating unsigned).
+  //

ebevhan wrote:
> leonardchan wrote:
> > If this is exclusively for codegen purposes with binary operations, would 
> > it be clearer to move this to `EmitFixedPointBinOp`? If 
> > `UnsignedPaddingIsSigned` doesn't need to be used for stuff like constant 
> > evaluation, it might be clearer not to provide it for everyone.
> FixedPointSemantics is immutable except for saturation, unfortunately. I'd 
> end up having to reconstruct the semantics object from scratch immediately 
> after calling getCommonSemantics.
Fair.

Minor nit: Could you rename the parameter to `UnsignedPaddingIsSignedForCG`? 
Just want to make it clearer that this should specifically be used for codegen 
only.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In D82663#2140507 , @leonardchan wrote:

> In D82663#2130355 , @ebevhan wrote:
>
> > Well, it's not so much as adding the bit, but adding the information that 
> > the bit exists. That means either new intrinsics for all of the operations, 
> > or adding flags to the existing ones. That's a fair bit of added 
> > complexity. Also,  +  would do virtually 
> > the exact same thing as the new unsigned-with-padding operations, so the 
> > utility of adding all of it is a bit questionable.
>
>
> Could the work involved just be adding the flags, then in 
> `llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp` for unsigned 
> operations, choosing between the signed/unsigned underlying `ISD` when 
> lowering intrinsics to DAG? I think you could just pass the padding bit to 
> `FixedPointIntrinsicToOpcode` and handle it from there. This is just off the 
> top of my head so I could be missing other things.


It wouldn't just be restricted to fixed-point intrinsics, though. It would have 
to be added to intrinsics like uadd.sat and usub.sat as well, which aren't 
really tied to fixed-point at all. Changing the semantics of those intrinsics 
would be unfortunate for targets that have started using them for their own 
instructions. I don't really think it's an option to move the padding semantic 
info into the IR; the intrinsic interface is fairly lean, and I think keeping 
it that way is a good idea.

I could change the emission to not be so heavy-handed and only use signed 
operations for the intrinsics, rather than everything. That makes 
EmitFixedPointBinOp a bit messier, though.




Comment at: clang/lib/Basic/FixedPoint.cpp:143-158
+  // For codegen purposes, make unsigned with padding semantics signed instead.
+  // This means that we will generate signed operations. The result from these
+  // operations is defined, since ending up with a negative result is undefined
+  // for nonsaturating semantics, and for saturating semantics we will
+  // perform a clamp-to-zero in the last conversion to result semantics (since
+  // we are going from saturating signed to saturating unsigned).
+  //

leonardchan wrote:
> If this is exclusively for codegen purposes with binary operations, would it 
> be clearer to move this to `EmitFixedPointBinOp`? If 
> `UnsignedPaddingIsSigned` doesn't need to be used for stuff like constant 
> evaluation, it might be clearer not to provide it for everyone.
FixedPointSemantics is immutable except for saturation, unfortunately. I'd end 
up having to reconstruct the semantics object from scratch immediately after 
calling getCommonSemantics.



Comment at: clang/test/Frontend/fixed_point_add.c:290-294
+  // UNSIGNED-NEXT: [[USA_EXT:%[a-z0-9]+]] = zext i16 [[USA]] to i40
+  // UNSIGNED-NEXT: [[I_EXT:%[a-z0-9]+]] = zext i32 [[I]] to i40
+  // UNSIGNED-NEXT: [[I:%[a-z0-9]+]] = shl i40 [[I_EXT]], 7
+  // UNSIGNED-NEXT: [[SUM:%[0-9]+]] = add i40 [[USA_EXT]], [[I]]
+  // UNSIGNED-NEXT: [[RES:%[a-z0-9]+]] = trunc i40 [[SUM]] to i16

leonardchan wrote:
> If this is a workaround for not being able to convey the padding bit to LLVM 
> intrinsics, I think we should only limit changes to instances we would use 
> intrinsics.
I suppose this makes sense, but the logic will be a bit more convoluted in that 
case.

It is true that in most cases, the clamp-to-zero resulting from the 
signed->unsigned conversion at the end isn't even necessary. For addition, 
multiplication, division and shift, the result of positive operands can never 
become negative, so there's no point to the clamp.

It just felt more general to do it for all of them instead of littering 
EmitFixedPointBinOp with special cases. But perhaps it would be better to deal 
with each case individually instead. Still feels like that would make the 
implementation less clean.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

In D82663#2130355 , @ebevhan wrote:

> Well, it's not so much as adding the bit, but adding the information that the 
> bit exists. That means either new intrinsics for all of the operations, or 
> adding flags to the existing ones. That's a fair bit of added complexity. 
> Also,  +  would do virtually the exact same 
> thing as the new unsigned-with-padding operations, so the utility of adding 
> all of it is a bit questionable.


Could the work involved just be adding the flags, then in 
`llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp` for unsigned 
operations, choosing between the signed/unsigned underlying `ISD` when lowering 
intrinsics to DAG? I think you could just pass the padding bit to 
`FixedPointIntrinsicToOpcode` and handle it from there. This is just off the 
top of my head so I could be missing other things.

I don't think this is necessarily the same as "legalizing" the intrinsic, but 
this would at least prevent frontends from second-guessing.




Comment at: clang/include/clang/Basic/FixedPoint.h:66
   /// given binary operation.
+  /// Is UnsignedPaddingIsSigned is true, unsigned semantics which would
+  /// otherwise have been unsigned will be signed instead. This is for codegen

Is -> If



Comment at: clang/lib/Basic/FixedPoint.cpp:143-158
+  // For codegen purposes, make unsigned with padding semantics signed instead.
+  // This means that we will generate signed operations. The result from these
+  // operations is defined, since ending up with a negative result is undefined
+  // for nonsaturating semantics, and for saturating semantics we will
+  // perform a clamp-to-zero in the last conversion to result semantics (since
+  // we are going from saturating signed to saturating unsigned).
+  //

If this is exclusively for codegen purposes with binary operations, would it be 
clearer to move this to `EmitFixedPointBinOp`? If `UnsignedPaddingIsSigned` 
doesn't need to be used for stuff like constant evaluation, it might be clearer 
not to provide it for everyone.



Comment at: clang/test/Frontend/fixed_point_add.c:290-294
+  // UNSIGNED-NEXT: [[USA_EXT:%[a-z0-9]+]] = zext i16 [[USA]] to i40
+  // UNSIGNED-NEXT: [[I_EXT:%[a-z0-9]+]] = zext i32 [[I]] to i40
+  // UNSIGNED-NEXT: [[I:%[a-z0-9]+]] = shl i40 [[I_EXT]], 7
+  // UNSIGNED-NEXT: [[SUM:%[0-9]+]] = add i40 [[USA_EXT]], [[I]]
+  // UNSIGNED-NEXT: [[RES:%[a-z0-9]+]] = trunc i40 [[SUM]] to i16

If this is a workaround for not being able to convey the padding bit to LLVM 
intrinsics, I think we should only limit changes to instances we would use 
intrinsics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan updated this revision to Diff 275657.
ebevhan added a comment.

Rebased.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82663

Files:
  clang/include/clang/Basic/FixedPoint.h
  clang/lib/Basic/FixedPoint.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/test/Frontend/fixed_point_add.c
  clang/test/Frontend/fixed_point_comparisons.c
  clang/test/Frontend/fixed_point_compound.c
  clang/test/Frontend/fixed_point_div.c
  clang/test/Frontend/fixed_point_mul.c
  clang/test/Frontend/fixed_point_sub.c
  clang/test/Frontend/fixed_point_unary.c

Index: clang/test/Frontend/fixed_point_unary.c
===
--- clang/test/Frontend/fixed_point_unary.c
+++ clang/test/Frontend/fixed_point_unary.c
@@ -68,28 +68,28 @@
 // CHECK: [[TMP18:%.*]] = load i32, i32* @sua, align 4
 // SIGNED-NEXT:   [[TMP19:%.*]] = call i32 @llvm.uadd.sat.i32(i32 [[TMP18]], i32 65536)
 // SIGNED-NEXT:   store i32 [[TMP19]], i32* @sua, align 4
-// UNSIGNED-NEXT: [[RESIZE:%.*]] = trunc i32 [[TMP18]] to i31
-// UNSIGNED-NEXT: [[TMP19:%.*]] = call i31 @llvm.uadd.sat.i31(i31 [[RESIZE]], i31 32768)
-// UNSIGNED-NEXT: [[RESIZE1:%.*]] = zext i31 [[TMP19]] to i32
-// UNSIGNED-NEXT: store i32 [[RESIZE1]], i32* @sua, align 4
+// UNSIGNED-NEXT: [[TMP19:%.*]] = call i32 @llvm.sadd.sat.i32(i32 [[TMP18]], i32 32768)
+// UNSIGNED-NEXT: [[TMP20:%.*]] = icmp slt i32 [[TMP19]], 0
+// UNSIGNED-NEXT: [[SATMIN:%.*]] = select i1 [[TMP20]], i32 0, i32 [[TMP19]]
+// UNSIGNED-NEXT: store i32 [[SATMIN]], i32* @sua, align 4
   sua++;
 
 // CHECK: [[TMP20:%.*]] = load i16, i16* @susa, align 2
 // SIGNED-NEXT:   [[TMP21:%.*]] = call i16 @llvm.uadd.sat.i16(i16 [[TMP20]], i16 256)
 // SIGNED-NEXT:   store i16 [[TMP21]], i16* @susa, align 2
-// UNSIGNED-NEXT: [[RESIZE2:%.*]] = trunc i16 [[TMP20]] to i15
-// UNSIGNED-NEXT: [[TMP21:%.*]] = call i15 @llvm.uadd.sat.i15(i15 [[RESIZE2]], i15 128)
-// UNSIGNED-NEXT: [[RESIZE3:%.*]] = zext i15 [[TMP21]] to i16
-// UNSIGNED-NEXT: store i16 [[RESIZE3]], i16* @susa, align 2
+// UNSIGNED-NEXT: [[TMP22:%.*]] = call i16 @llvm.sadd.sat.i16(i16 [[TMP20]], i16 128)
+// UNSIGNED-NEXT: [[TMP23:%.*]] = icmp slt i16 [[TMP22]], 0
+// UNSIGNED-NEXT: [[SATMIN1:%.*]] = select i1 [[TMP23]], i16 0, i16 [[TMP22]]
+// UNSIGNED-NEXT: store i16 [[SATMIN1]], i16* @susa, align 2
   susa++;
 
 // CHECK: [[TMP22:%.*]] = load i16, i16* @suf, align 2
 // SIGNED-NEXT:   [[TMP23:%.*]] = call i16 @llvm.uadd.sat.i16(i16 [[TMP22]], i16 -1)
 // SIGNED-NEXT:   store i16 [[TMP23]], i16* @suf, align 2
-// UNSIGNED-NEXT: [[RESIZE4:%.*]] = trunc i16 [[TMP22]] to i15
-// UNSIGNED-NEXT: [[TMP23:%.*]] = call i15 @llvm.uadd.sat.i15(i15 [[RESIZE4]], i15 -1)
-// UNSIGNED-NEXT: [[RESIZE5:%.*]] = zext i15 [[TMP23]] to i16
-// UNSIGNED-NEXT: store i16 [[RESIZE5]], i16* @suf, align 2
+// UNSIGNED-NEXT: [[TMP25:%.*]] = call i16 @llvm.sadd.sat.i16(i16 [[TMP22]], i16 32767)
+// UNSIGNED-NEXT: [[TMP26:%.*]] = icmp slt i16 [[TMP25]], 0
+// UNSIGNED-NEXT: [[SATMIN2:%.*]] = select i1 [[TMP26]], i16 0, i16 [[TMP25]]
+// UNSIGNED-NEXT: store i16 [[SATMIN2]], i16* @suf, align 2
   suf++;
 }
 
@@ -146,28 +146,28 @@
 // CHECK: [[TMP18:%.*]] = load i32, i32* @sua, align 4
 // SIGNED-NEXT:   [[TMP19:%.*]] = call i32 @llvm.usub.sat.i32(i32 [[TMP18]], i32 65536)
 // SIGNED-NEXT:   store i32 [[TMP19]], i32* @sua, align 4
-// UNSIGNED-NEXT: [[RESIZE:%.*]] = trunc i32 [[TMP18]] to i31
-// UNSIGNED-NEXT: [[TMP19:%.*]] = call i31 @llvm.usub.sat.i31(i31 [[RESIZE]], i31 32768)
-// UNSIGNED-NEXT: [[RESIZE1:%.*]] = zext i31 [[TMP19]] to i32
-// UNSIGNED-NEXT: store i32 [[RESIZE1]], i32* @sua, align 4
+// UNSIGNED-NEXT: [[TMP19:%.*]] = call i32 @llvm.ssub.sat.i32(i32 [[TMP18]], i32 32768)
+// UNSIGNED-NEXT: [[TMP20:%.*]] = icmp slt i32 [[TMP19]], 0
+// UNSIGNED-NEXT: [[SATMIN:%.*]] = select i1 [[TMP20]], i32 0, i32 [[TMP19]]
+// UNSIGNED-NEXT: store i32 [[SATMIN]], i32* @sua, align 4
   sua--;
 
 // CHECK: [[TMP20:%.*]] = load i16, i16* @susa, align 2
 // SIGNED-NEXT:   [[TMP21:%.*]] = call i16 @llvm.usub.sat.i16(i16 [[TMP20]], i16 256)
 // SIGNED-NEXT:   store i16 [[TMP21]], i16* @susa, align 2
-// UNSIGNED-NEXT: [[RESIZE2:%.*]] = trunc i16 [[TMP20]] to i15
-// UNSIGNED-NEXT: [[TMP21:%.*]] = call i15 @llvm.usub.sat.i15(i15 [[RESIZE2]], i15 128)
-// UNSIGNED-NEXT: [[RESIZE3:%.*]] = zext i15 [[TMP21]] to i16
-// UNSIGNED-NEXT: store i16 [[RESIZE3]], i16* @susa, align 2
+// UNSIGNED-NEXT: [[TMP22:%.*]] = call i16 @llvm.ssub.sat.i16(i16 [[TMP20]], i16 128)
+// UNSIGNED-NEXT: [[TMP23:%.*]] = icmp slt i16 [[TMP22]], 0
+// UNSIGNED-NEXT: [[SATMIN1:%.*]] = select i1 [[TMP23]], i16 0, i16 [[TMP22]]
+// UNSIGNED-NEXT: store i16 [[SATMIN1]], i16* @susa, align 2
   susa--;
 
 // CHECK: [[TMP22:%.*]] = load i16, i16* @suf, align 2
 // SIGNED-NEXT:   [[TMP23:%.*]] = call i16 @llvm.usub.sat.i16(i16 [[TMP22]], i16 -1)
 // SIGNED-NEXT: 

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

Well, it's not so much as adding the bit, but adding the information that the 
bit exists. That means either new intrinsics for all of the operations, or 
adding flags to the existing ones. That's a fair bit of added complexity. Also, 
 +  would do virtually the exact same thing as 
the new unsigned-with-padding operations, so the utility of adding all of it is 
a bit questionable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Can the missing bit just be added?  It seems to me that frontends ought to be 
able to emit the obvious intrinsic for the semantic operation here rather than 
having to second-guess the backend.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

Another point is, if we for example have an i16 umul.fix in legalization, we 
have no way of knowing in the general case that it is safe to replace it with 
an smul.fix, since the information that the MSB is not significant does not 
exist on IR level. This is the 'loss of padding bit' that I'm referring to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In D82663#2117451 , @rjmccall wrote:

> Why not legalize to the signed operation?


My feeling was that it wasn't right do so in LLVM, because LLVM has no notion 
of the padding bit and therefore doesn't really care about Clang's rationale 
for doing the legalization that way.
If an illegal udiv.fix.sat needs to be legalized via promotion, it makes more 
sense to legalize it to another unsigned operation rather than arbitrarily 
doing it as a signed one.

I guess it could be done in cases where the resulting signed operation was 
legal and the unsigned one was not, but that isn't testable upstream since none 
of the operations are legal on upstream targets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Why not legalize to the signed operation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan updated this revision to Diff 273762.
ebevhan added a comment.

Fixed some broken CHECK lines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82663

Files:
  clang/include/clang/Basic/FixedPoint.h
  clang/lib/Basic/FixedPoint.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/test/Frontend/fixed_point_add.c
  clang/test/Frontend/fixed_point_comparisons.c
  clang/test/Frontend/fixed_point_div.c
  clang/test/Frontend/fixed_point_mul.c
  clang/test/Frontend/fixed_point_sub.c
  clang/test/Frontend/fixed_point_unary.c

Index: clang/test/Frontend/fixed_point_unary.c
===
--- clang/test/Frontend/fixed_point_unary.c
+++ clang/test/Frontend/fixed_point_unary.c
@@ -68,28 +68,28 @@
 // CHECK: [[TMP18:%.*]] = load i32, i32* @sua, align 4
 // SIGNED-NEXT:   [[TMP19:%.*]] = call i32 @llvm.uadd.sat.i32(i32 [[TMP18]], i32 65536)
 // SIGNED-NEXT:   store i32 [[TMP19]], i32* @sua, align 4
-// UNSIGNED-NEXT: [[RESIZE:%.*]] = trunc i32 [[TMP18]] to i31
-// UNSIGNED-NEXT: [[TMP19:%.*]] = call i31 @llvm.uadd.sat.i31(i31 [[RESIZE]], i31 32768)
-// UNSIGNED-NEXT: [[RESIZE1:%.*]] = zext i31 [[TMP19]] to i32
-// UNSIGNED-NEXT: store i32 [[RESIZE1]], i32* @sua, align 4
+// UNSIGNED-NEXT: [[TMP19:%.*]] = call i32 @llvm.sadd.sat.i32(i32 [[TMP18]], i32 32768)
+// UNSIGNED-NEXT: [[TMP20:%.*]] = icmp slt i32 [[TMP19]], 0
+// UNSIGNED-NEXT: [[SATMIN:%.*]] = select i1 [[TMP20]], i32 0, i32 [[TMP19]]
+// UNSIGNED-NEXT: store i32 [[SATMIN]], i32* @sua, align 4
   sua++;
 
 // CHECK: [[TMP20:%.*]] = load i16, i16* @susa, align 2
 // SIGNED-NEXT:   [[TMP21:%.*]] = call i16 @llvm.uadd.sat.i16(i16 [[TMP20]], i16 256)
 // SIGNED-NEXT:   store i16 [[TMP21]], i16* @susa, align 2
-// UNSIGNED-NEXT: [[RESIZE2:%.*]] = trunc i16 [[TMP20]] to i15
-// UNSIGNED-NEXT: [[TMP21:%.*]] = call i15 @llvm.uadd.sat.i15(i15 [[RESIZE2]], i15 128)
-// UNSIGNED-NEXT: [[RESIZE3:%.*]] = zext i15 [[TMP21]] to i16
-// UNSIGNED-NEXT: store i16 [[RESIZE3]], i16* @susa, align 2
+// UNSIGNED-NEXT: [[TMP22:%.*]] = call i16 @llvm.sadd.sat.i16(i16 [[TMP20]], i16 128)
+// UNSIGNED-NEXT: [[TMP23:%.*]] = icmp slt i16 [[TMP22]], 0
+// UNSIGNED-NEXT: [[SATMIN1:%.*]] = select i1 [[TMP23]], i16 0, i16 [[TMP22]]
+// UNSIGNED-NEXT: store i16 [[SATMIN1]], i16* @susa, align 2
   susa++;
 
 // CHECK: [[TMP22:%.*]] = load i16, i16* @suf, align 2
 // SIGNED-NEXT:   [[TMP23:%.*]] = call i16 @llvm.uadd.sat.i16(i16 [[TMP22]], i16 -1)
 // SIGNED-NEXT:   store i16 [[TMP23]], i16* @suf, align 2
-// UNSIGNED-NEXT: [[RESIZE4:%.*]] = trunc i16 [[TMP22]] to i15
-// UNSIGNED-NEXT: [[TMP23:%.*]] = call i15 @llvm.uadd.sat.i15(i15 [[RESIZE4]], i15 -1)
-// UNSIGNED-NEXT: [[RESIZE5:%.*]] = zext i15 [[TMP23]] to i16
-// UNSIGNED-NEXT: store i16 [[RESIZE5]], i16* @suf, align 2
+// UNSIGNED-NEXT: [[TMP25:%.*]] = call i16 @llvm.sadd.sat.i16(i16 [[TMP22]], i16 32767)
+// UNSIGNED-NEXT: [[TMP26:%.*]] = icmp slt i16 [[TMP25]], 0
+// UNSIGNED-NEXT: [[SATMIN2:%.*]] = select i1 [[TMP26]], i16 0, i16 [[TMP25]]
+// UNSIGNED-NEXT: store i16 [[SATMIN2]], i16* @suf, align 2
   suf++;
 }
 
@@ -146,28 +146,28 @@
 // CHECK: [[TMP18:%.*]] = load i32, i32* @sua, align 4
 // SIGNED-NEXT:   [[TMP19:%.*]] = call i32 @llvm.usub.sat.i32(i32 [[TMP18]], i32 65536)
 // SIGNED-NEXT:   store i32 [[TMP19]], i32* @sua, align 4
-// UNSIGNED-NEXT: [[RESIZE:%.*]] = trunc i32 [[TMP18]] to i31
-// UNSIGNED-NEXT: [[TMP19:%.*]] = call i31 @llvm.usub.sat.i31(i31 [[RESIZE]], i31 32768)
-// UNSIGNED-NEXT: [[RESIZE1:%.*]] = zext i31 [[TMP19]] to i32
-// UNSIGNED-NEXT: store i32 [[RESIZE1]], i32* @sua, align 4
+// UNSIGNED-NEXT: [[TMP19:%.*]] = call i32 @llvm.ssub.sat.i32(i32 [[TMP18]], i32 32768)
+// UNSIGNED-NEXT: [[TMP20:%.*]] = icmp slt i32 [[TMP19]], 0
+// UNSIGNED-NEXT: [[SATMIN:%.*]] = select i1 [[TMP20]], i32 0, i32 [[TMP19]]
+// UNSIGNED-NEXT: store i32 [[SATMIN]], i32* @sua, align 4
   sua--;
 
 // CHECK: [[TMP20:%.*]] = load i16, i16* @susa, align 2
 // SIGNED-NEXT:   [[TMP21:%.*]] = call i16 @llvm.usub.sat.i16(i16 [[TMP20]], i16 256)
 // SIGNED-NEXT:   store i16 [[TMP21]], i16* @susa, align 2
-// UNSIGNED-NEXT: [[RESIZE2:%.*]] = trunc i16 [[TMP20]] to i15
-// UNSIGNED-NEXT: [[TMP21:%.*]] = call i15 @llvm.usub.sat.i15(i15 [[RESIZE2]], i15 128)
-// UNSIGNED-NEXT: [[RESIZE3:%.*]] = zext i15 [[TMP21]] to i16
-// UNSIGNED-NEXT: store i16 [[RESIZE3]], i16* @susa, align 2
+// UNSIGNED-NEXT: [[TMP22:%.*]] = call i16 @llvm.ssub.sat.i16(i16 [[TMP20]], i16 128)
+// UNSIGNED-NEXT: [[TMP23:%.*]] = icmp slt i16 [[TMP22]], 0
+// UNSIGNED-NEXT: [[SATMIN1:%.*]] = select i1 [[TMP23]], i16 0, i16 [[TMP22]]
+// UNSIGNED-NEXT: store i16 [[SATMIN1]], i16* @susa, align 2
   susa--;
 
 // CHECK: [[TMP22:%.*]] = load i16, i16* @suf, align 2
 // SIGNED-NEXT:   [[TMP23:%.*]] = call i16 @llvm.usub.sat.i16(i16 [[TMP22]], i16 -1)
 // SIGNED-NEXT:   store i16 [[TMP23]], 

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan created this revision.
ebevhan added reviewers: leonardchan, rjmccall, bjope.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The design of unsigned fixed-point with padding did not really
work as originally intended downstream.

The issue with the design is that the concept of the unsigned
padding bit disappears in the transition to IR. On the LLVM
level, there is no padding bit and anything goes with the
operations. This has the unfortunate effect of generating
invalid operations during ISel for operations that a target
should be perfectly capable of selecting for.

For example, for an unsigned saturating _Fract division of
width 16, we emit IR for an i15 udiv.fix.sat. In the legalization
of this operation in ISel, the operand and result are promoted
to i16, and to preserve the saturating behavior, the LHS is
shifted left by 1.

However... This means that we now have a division operation
with a significant value in the LHS MSB. If the target could
select this, there would be no meaning to the padding bit.
Considering that ISel will always promote this due to type
illegality, there's no way around the production of illegal
operations.

This patch changes CodeGen to emit signed operations when
emitting code for unsigned with padding. At least for us
downstream, being able to reuse the signed instructions is
the one of the points of having the padding bit, so this
design seems to align better.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82663

Files:
  clang/include/clang/Basic/FixedPoint.h
  clang/lib/Basic/FixedPoint.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/test/Frontend/fixed_point_add.c
  clang/test/Frontend/fixed_point_comparisons.c
  clang/test/Frontend/fixed_point_div.c
  clang/test/Frontend/fixed_point_mul.c
  clang/test/Frontend/fixed_point_sub.c
  clang/test/Frontend/fixed_point_unary.c

Index: clang/test/Frontend/fixed_point_unary.c
===
--- clang/test/Frontend/fixed_point_unary.c
+++ clang/test/Frontend/fixed_point_unary.c
@@ -68,28 +68,28 @@
 // CHECK: [[TMP18:%.*]] = load i32, i32* @sua, align 4
 // SIGNED-NEXT:   [[TMP19:%.*]] = call i32 @llvm.uadd.sat.i32(i32 [[TMP18]], i32 65536)
 // SIGNED-NEXT:   store i32 [[TMP19]], i32* @sua, align 4
-// UNSIGNED-NEXT: [[RESIZE:%.*]] = trunc i32 [[TMP18]] to i31
-// UNSIGNED-NEXT: [[TMP19:%.*]] = call i31 @llvm.uadd.sat.i31(i31 [[RESIZE]], i31 32768)
-// UNSIGNED-NEXT: [[RESIZE1:%.*]] = zext i31 [[TMP19]] to i32
-// UNSIGNED-NEXT: store i32 [[RESIZE1]], i32* @sua, align 4
+// UNSIGNED-NEXT: [[TMP19:%.*]] = call i32 @llvm.sadd.sat.i32(i32 [[TMP18]], i32 32768)
+// UNSIGNED-NEXT: [[TMP20:%.*]] = icmp slt i32 [[TMP19]], 0
+// UNSIGNED-NEXT: [[SATMIN:%.*]] = select i1 [[TMP20]], i32 0, i32 [[TMP19]]
+// UNSIGNED-NEXT: store i32 [[SATMIN]], i32* @sua, align 4
   sua++;
 
 // CHECK: [[TMP20:%.*]] = load i16, i16* @susa, align 2
 // SIGNED-NEXT:   [[TMP21:%.*]] = call i16 @llvm.uadd.sat.i16(i16 [[TMP20]], i16 256)
 // SIGNED-NEXT:   store i16 [[TMP21]], i16* @susa, align 2
-// UNSIGNED-NEXT: [[RESIZE2:%.*]] = trunc i16 [[TMP20]] to i15
-// UNSIGNED-NEXT: [[TMP21:%.*]] = call i15 @llvm.uadd.sat.i15(i15 [[RESIZE2]], i15 128)
-// UNSIGNED-NEXT: [[RESIZE3:%.*]] = zext i15 [[TMP21]] to i16
-// UNSIGNED-NEXT: store i16 [[RESIZE3]], i16* @susa, align 2
+// UNSIGNED-NEXT: [[TMP22:%.*]] = call i16 @llvm.sadd.sat.i16(i16 [[TMP20]], i16 128)
+// UNSIGNED-NEXT: [[TMP23:%.*]] = icmp slt i16 [[TMP22]], 0
+// UNSIGNED-NEXT: [[SATMIN1:%.*]] = select i1 [[TMP23]], i16 0, i16 [[TMP22]]
+// UNSIGNED-NEXT: store i16 [[SATMIN1]], i16* @susa, align 2
   susa++;
 
 // CHECK: [[TMP22:%.*]] = load i16, i16* @suf, align 2
 // SIGNED-NEXT:   [[TMP23:%.*]] = call i16 @llvm.uadd.sat.i16(i16 [[TMP22]], i16 -1)
 // SIGNED-NEXT:   store i16 [[TMP23]], i16* @suf, align 2
-// UNSIGNED-NEXT: [[RESIZE4:%.*]] = trunc i16 [[TMP22]] to i15
-// UNSIGNED-NEXT: [[TMP23:%.*]] = call i15 @llvm.uadd.sat.i15(i15 [[RESIZE4]], i15 -1)
-// UNSIGNED-NEXT: [[RESIZE5:%.*]] = zext i15 [[TMP23]] to i16
-// UNSIGNED-NEXT: store i16 [[RESIZE5]], i16* @suf, align 2
+// UNSIGNED-NEXT: [[TMP25:%.*]] = call i16 @llvm.sadd.sat.i16(i16 [[TMP22]], i16 32767)
+// UNSIGNED-NEXT: [[TMP26:%.*]] = icmp slt i16 [[TMP25]], 0
+// UNSIGNED-NEXT: [[SATMIN2:%.*]] = select i1 [[TMP26]], i16 0, i16 [[TMP25]]
+// UNSIGNED-NEXT: store i16 [[SATMIN2]], i16* @suf, align 2
   suf++;
 }
 
@@ -146,28 +146,28 @@
 // CHECK: [[TMP18:%.*]] = load i32, i32* @sua, align 4
 // SIGNED-NEXT:   [[TMP19:%.*]] = call i32 @llvm.usub.sat.i32(i32 [[TMP18]], i32 65536)
 // SIGNED-NEXT:   store i32 [[TMP19]], i32* @sua, align 4
-// UNSIGNED-NEXT: [[RESIZE:%.*]] = trunc i32 [[TMP18]] to i31
-// UNSIGNED-NEXT: [[TMP19:%.*]] = call i31 @llvm.usub.sat.i31(i31 [[RESIZE]], i31 32768)
-// UNSIGNED-NEXT: [[RESIZE1:%.*]] = zext i31 [[TMP19]] to i32
-// UNSIGNED-NEXT: store i32 [[RESIZE1]], i32* @sua, align 4