Is this patch good to land?

This patch fixes a Windows Visual Studio build lit issue introduced by rL306895 (D34855).

In D34855#798200, @grimar wrote:

Ah, I just noticed the D34853, it fixes the issue I am observing.

I believe that this "build mode" is intended for the Visual Studio MSVC build. This build is special in that it can produce builds for multiple configurations, e.g. Debug, Release & RelWithDebInfo, within the same top level build output directory. It is this configuration type that defines the "build mode". This means that the unit tests will only pick up the configuration that matches that of the lit that was run. Without the "build mode" I believe lit might end up running all configurations of unit tests that have been built, which is probably not the intended behaviour.

In D34204#790836, @andrewng wrote:

Hi Rafael,

Is this patch now good to land?

Cheers,
Andrew

Hi Rafael,

Updated for review comments.

Is there any more feedback on this patch or is it good to land?

In D34204#786240, @ruiu wrote:

Can you change your program?

In D34204#784448, @ruiu wrote:

Hi Andrew,

What are you actually trying to address with this patch? I'd like to know more about the background as to why you want this. Is there any reason you can't fix your program rather than the linker?

Is this patch good to land?

In D32910#747152, @zturner wrote:

I don't disbelieve you, but do you have any thoughts about why I've never seen this when running tests locally?

As suggested by Adrian, I have made prependDIExpr a static member function of DIExpression (and also appendOffset used by prependDIExpr). This is then used to apply the LEA address shift to the replacement debug value.

Sorry for the slight delay but only just got back from holiday.

In D31755#731245, @aprantl wrote:

Rather than scanning forward for DBG_VALUEs, is there a more systematic way to find them? E.g., by iterating over USEs? If not, doing it this way is probably fine.

Updated test to focus only on the instcombine pass.

I've updated the comments to explicitly mention the MinGW-w64 case that encounters this issue and removed the unnecessary FunctionType related checks.

In D31806#722732, @efriedma wrote:

gcc transforms the given C code to a call to expf(), just like LLVM, but then it looks like their inliner works a bit differently so the generated code ends up with a call to expf() rather than an infinite loop. With your patch, LLVM also generates a call to expf(). This seems weird... does MinGW's C library actually have an expf() function?

In D31806#721631, @efriedma wrote:

Can you explain a little more how exactly we end up in this situation? Your example has undefined behavior according to the C standard.

Removed DWARF version checks as suggested by Adrian (thanks) and updated comments.

Thanks for the review.

I have now split this patch and updated this review to contain the first part which fixes the codegen issue but doesn't preserve the debug values. I will work on a separate patch which will attempt to preserve the debug values.

In D30835#699193, @andrewng wrote:

In D30835#697996, @andrewng wrote:

In D30835#697933, @dblaikie wrote:

Looks like this change does two things - fixes the "debug info affects optimization" and also does some work to improve optimized debug info quality? Might be worth separating this into two changes to make sure there's good test coverage/easier to review/etc?

I'm about to leave work now but I will look on Monday into whether separating this into two makes sense.

On further consideration, I think it makes sense to keep the patch as is because otherwise the first patch would be "lossy", i.e. the output would lose debug information. So in fixing the optimisation, ideally the debug information should be correct and preserved. However, if there are concerns with the "patching" of the debug information, then it might make sense to split the patch. What are your thoughts?

In D30835#697996, @andrewng wrote:

In D30835#697933, @dblaikie wrote:

Looks like this change does two things - fixes the "debug info affects optimization" and also does some work to improve optimized debug info quality? Might be worth separating this into two changes to make sure there's good test coverage/easier to review/etc?

I'm about to leave work now but I will look on Monday into whether separating this into two makes sense.

In D30835#697933, @dblaikie wrote:

Looks like this change does two things - fixes the "debug info affects optimization" and also does some work to improve optimized debug info quality? Might be worth separating this into two changes to make sure there's good test coverage/easier to review/etc?