In D53340#1267393, @bjope wrote:

Is this intrinsic really needed for the fixed point support?

Maybe I've missed something in the discussions about support for unsigned fixed point types, but in our implementation the unsigned types are using the positive range of the signed type, such as [0, SIGNED_MAX]. So we keep the sign bit zero for the unsigned fixed point types. Are you planning for something different here?

In our implementation we lower addition of two saturated unsigned fixed point types to sadd.sat (if the input values are in the range [0, SIGNED_MAX] the result will be in the range [0, SIGNED_MAX] as well). So we haven't really found a need for implementing uadd.sat.

@philip.pfaffe @fedor.sergeev Do you have any more comments on this patch?

Sorry for the delay. Made patch https://reviews.llvm.org/D53299. Not
sure if I needed to make a patch for this or can commit right away,
but wanted to run it by you first.

I found those earlier when working on the monorepo. In fixing them now.

In D52814#1262550, @philip.pfaffe wrote:

You're right, my bad; I missed the fact that it's EP_OptimizerLast, which has no equivalent in the new PM. Your implementation is actually correct here.

Did you run your test in a debug build?

In D53053#1263960, @craig.topper wrote:

Intrinsics with vector operands are expanded in LegalizeVectorOps.cpp. Sometimes with a vectorized sequence or in the worst case by unrolling.

• Removed target hook

In D53053#1263049, @craig.topper wrote:

Are we not supporting vectors at all? That needs to be checked in the IR verifier if so. Though I would really like to see this for vectors since X86 has 8 and 16 bit saturating vector addition.

@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?

In D52286#1258497, @bjope wrote:

If the use of a special intrinsic for saturation is rare (few targets that supports it natively), then perhaps we should put this particular patch on hold for now?

I guess the target that @bevinh (and I) is working on can override the codegen in clang to select our target specific intrinsic just like we do today. Or is it hard to do target specific overrides in clang codegen?

The generic support for fixed point types could, at least initially, use the min/max clamping. It would let us move forward on the more interesting parts of fixed point support (such as saturated add, multiplication etc), instead of getting stuck in heavy discussions regarding something that IMHO has lower priority. Then we can revisit this patch later. Perhaps it will be easier to demonstrate the need for an ssaturate intrinsic when we come further and clang is generating fixed point conversions.

These are different per instance though. Even if merged, you're free to create one instance with options A and one instance with options B and run the first instance on a function pipeline and the second on a module pipeline.

I meant just merging the passes. In the sense that you have one pass class with one module run() method and one function run() method. The question then is whether you'll ever want to use different options for the different IRUnits.

In D52739#1258087, @philip.pfaffe wrote:

Some comments inline. Does it make sense to merge the module and function passes into one? Is it really necessary to distinguish the passes?

In D52739#1257533, @philip.pfaffe wrote:

See my latest comment. If there is no state to be kept, that state need not be on the exported interface.

@ebevhan @craig.topper Any more comments on this patch?

Are there any other changes that would be necessary before getting revision acceptance?

This sounds like something that would be better handled in the build system rather than in source code. In particular, I think you don't actually want to detect "is this translation unit being compiled with ubsan enabled?", you instead want to detect "is the rest of musl libc being compiled with ubsan enabled?" -- you should not compile the stubs themselves with ubsan enabled, because ubsan might insert calls to its runtime at arbitrary places within that translation unit, which means you'll get infinite recursion when one of the ubsan callbacks ends up calling itself.

In D52739#1254912, @philip.pfaffe wrote:

Well, it will be a bit tricky to avoid moving the interfaces around, since AddressSanitizer contains quite a load of state

What kind of state is asan tracking that persists across functions or modules?

In D52814#1254901, @philip.pfaffe wrote:

Is this the right place in the pipeline to put the passes? The legacy PM inserts the asan passes at EP_OptimizerLast, why not do the same thing?

In D52386#1254437, @rsmith wrote:

I'm not at all convinced that this is a good thing. There isn't such a thing as "undefined behavior sanitizer". Rather, there are a whole bunch of different checks that fall under the same umbrella. This test seems on the surface to be about as meaningless as __has_feature(warnings) would be: it's useless to ask the question without knowing *which* warnings you're talking about. But perhaps there's some use case I've overlooked (and your description of the patch doesn't mention why you want this). What is the use case you're trying to address with this change?

In D52286#1249759, @craig.topper wrote:

Adding @spatel. This feels like it could be implemented with a min+max sequence in IR using 2 icmps and 2 selects. We've already spent quite a lot of effort optimizing these common patterns in both IR and SelectionDAG with the SMAX/SMIN ISD opcodes.

• Changed second argument of SSAT node from ConstantSDNode to VTSDNode

• Reduce size of patch by adding per-PM wrappers for AddressSanitizer and AddressSanitizerModule. The passes exposed to the new PM use opaque pointers to these wrappers.

• Added LLVM header description