In D53738#1358249, @rjmccall wrote:

In D53738#1333372, @rjmccall wrote:

In D53738#1333276, @leonardchan wrote:

In D53738#1326071, @rjmccall wrote:

I'm fine with making this change under the assumption that we've gotten the language rule right. Even if that weren't abstractly reasonable for general language work — and I do think it's reasonable when we have a good-faith question about the right semantics — this is clearly still an experimental implementation and will be for several months yet, and hopefully it won't take that long for us to get a response.

@rjmccall Have you received a response yet? If not, do you think you have an estimate on the response time, or also mind sharing the contact information if that's ok?

I just have a coworker who's part of the committee. I think you might be over-opimistic about how quickly things get answered with the C committee, though. We should not allow our work to be blocked waiting for a response.

The committee was less helpful than I hoped, but what I decided to take away from their response was that we should not artificially lose precision here by separating the signedness conversion from the operation.

In D55868#1358208, @rjmccall wrote:

Yeah, I would recommend splitting the APFixedPoint in APValue changes into a separate patch.

Regarding tests, I couldn't actually find any under clang/unittests that test APValue specifically. Is it worth it to add some still?

I ended up adding a lot to this patch, so feel free to let me know if I should split this up if it's more difficult to review.

Any more comments on this patch?

In D55720#1336355, @ebevhan wrote:

Nothing sticks out to me, so I think it looks good. Hard to tell if there are any sneaky edge cases in the lowering steps, though.

Maybe you should rebase this on top of the unsigned patch, since they're touching all the same places. Or are you waiting for it to land?

In D56470#1350722, @chandlerc wrote:

I think a key thing that this is currently missing (or I'm missing it when looking at hte patch) the core shift that we made working through how to do sanitizer stuff with MSan.

Specifically, I suspect that we don't need any analysis layer, and the module and function layer can operate entirely independently. This would require roughly the following changes to the current ASan infrastructure:

Posted second attempt at porting ASan using an analysis at D56470.

In D54337#1345024, @philip.pfaffe wrote:

@leonardchan Are you making any progress here?

In D53738#1326071, @rjmccall wrote:

I'm fine with making this change under the assumption that we've gotten the language rule right. Even if that weren't abstractly reasonable for general language work — and I do think it's reasonable when we have a good-faith question about the right semantics — this is clearly still an experimental implementation and will be for several months yet, and hopefully it won't take that long for us to get a response.

In D55625#1331183, @ebevhan wrote:

Maybe a test with scale == bitwidth? Other than that it looks good to me.

Analysis can be any information which is not an IR itself. TargetLibraryInfo, DominatorTree, GlobalsMetadata... whatever.
The only problem that you might find when using your custom analysis from the pass will be similar to the current dichotomy of module & function ASan.
You can not *run* module analysis from within a function pass. It can only be run on a module level.

*ping* Any more comments on this patch?

• Added the CXX11 test

In D53738#1325998, @rjmccall wrote:

We sent the question out, but we haven't gotten a response yet. I think going forward under the idea that this just changes the type but does the operation on the original operand types is the right way to go forward in the short term.

In D53738#1320936, @rjmccall wrote:

Okay, thanks, that makes sense to me.

I'll ask around to find a way to contact the C committee.

*ping* @craig.topper @RKSimon Any more comments on this patch?

In D54719#1317563, @efriedma wrote:

You might need PromoteIntOp_SMULFIX for certain targets, like 64-bit RISCV.

*ping* @rsmith Any more comments on this patch or the one before it (https://reviews.llvm.org/D54014)? That one has the fix for pushing and popping another ExprEvalContext before acting on a function body in this patch.

@craig.topper @RKSimon @efriedma Any more comments on this patch?

*ping* Does anyone have any more comments on this patch?

It's certainly interesting to degenerate integer-with-fixedpoint to just a mul (since the scaling factor is 2^-n * 2^0, which is just 2^-n), but in the general case you can't avoid doing the scale alignment. Unless I'm missing something.