In D144565#4183779, @zhuna8616 wrote:

We intend this for llvm release/11.x version. Perhaps this can go as a backport?

I didn't see any tests in the PR. Please add one.

I have re-commit as suggested. Thank you for pointing out the testing issue for windows.

Add tests.

Add tests.

In D133480#3780651, @dblaikie wrote:

fair enough, if this is gold/binutils dwp's behavior (can you provide a pastebin/show this is the behavior there, maybe even point to the code that implements it?) doesn't seem like the worst thing to have in llvm-dwp, though this will need a test case

In D133480#3779118, @dblaikie wrote:

In D133480#3778767, @steven.zhang wrote:

In D133480#3778054, @dblaikie wrote:

In D133480#3777196, @steven.zhang wrote:

In D133480#3777059, @dblaikie wrote:

I don't think this is correct - the dwp tool might be run from a different directory than the equivalent comp_dir and so treating these as cwd-of-dwp-relative paths seems problematic.

I think the correct thing to do in the situation described is for the compilation to use -fdebug-compilation-dir to make the debug info portable - this will also ensure that a debugger can find the source and find the .dwo files even if they aren't packaged into a .dwp.

Can we do it as what gdb did ? That is, getting the dwo from absolute path first, and if failed, get it from relative path ?

I'm somewhat open to that, though I'd really encourage you to switch your build system/build flags so you don't have to keep adding functionality like that into other debug info consuming tools.

Thank for the suggestion, and I appreciate it. The issue of adding the build flags is that, it will pollute the remote cache. For example:

• building in workspace A: adding compilation option -fdebug-compilation-dir=path_to_workspace_A, and the objects will be cached in the remote build server.

• building in workspace B: adding compilation option -fdebug-compilation-dir=path_to_workspace_B, we cannot get the objects from remote cache though all the source code didn't change. We have to rebuild it as the compilation option changed.

If with relative path, we don't need to rebuild the objects as everything is unchanged. Remote cache is important to large project to speed up the building.

I will extend current implementation to support relative path.

Ah, sorry I didn't fully explain - yes (for context: I work at Google/on the compiler we use in the internal version of Bazel - so I'm familiar with the complexities of distributed and reusable build caches). Specifically, if the comp_dir is basically unusable (because if it's accurate/absolute for a local user, it's useless to another user, and it can't be whatever's on the build node either) you can use -fdebug-compilation-dir=. (I think at Google we use /proc/cwd to be precise) and then you'll get the current-working-directory relative behavior without consumers having to implement any special case.

Address comments.

In D133480#3778054, @dblaikie wrote:

In D133480#3777196, @steven.zhang wrote:

In D133480#3777059, @dblaikie wrote:

I don't think this is correct - the dwp tool might be run from a different directory than the equivalent comp_dir and so treating these as cwd-of-dwp-relative paths seems problematic.

I think the correct thing to do in the situation described is for the compilation to use -fdebug-compilation-dir to make the debug info portable - this will also ensure that a debugger can find the source and find the .dwo files even if they aren't packaged into a .dwp.

Can we do it as what gdb did ? That is, getting the dwo from absolute path first, and if failed, get it from relative path ?

I'm somewhat open to that, though I'd really encourage you to switch your build system/build flags so you don't have to keep adding functionality like that into other debug info consuming tools.

Thank for the suggestion, and I appreciate it. The issue of adding the build flags is that, it will pollute the remote cache. For example:

In D133480#3777059, @dblaikie wrote:

I don't think this is correct - the dwp tool might be run from a different directory than the equivalent comp_dir and so treating these as cwd-of-dwp-relative paths seems problematic.

I think the correct thing to do in the situation described is for the compilation to use -fdebug-compilation-dir to make the debug info portable - this will also ensure that a debugger can find the source and find the .dwo files even if they aren't packaged into a .dwp.

In D133480#3777059, @dblaikie wrote:

I don't think this is correct - the dwp tool might be run from a different directory than the equivalent comp_dir and so treating these as cwd-of-dwp-relative paths seems problematic.

I think the correct thing to do in the situation described is for the compilation to use -fdebug-compilation-dir to make the debug info portable - this will also ensure that a debugger can find the source and find the .dwo files even if they aren't packaged into a .dwp.

In D131771#3738758, @MarkGoncharovAl wrote:

I do not have commit access.
Someone who has it, please, push this commit to upstream.

In D91050#2917401, @efriedma wrote:

In D91050#2915306, @steven.zhang wrote:

In D91050#2905380, @efriedma wrote:

Passing an alignment in and returning an alignment out at the same time seems unlikely to be useful. DAGCombine can refine the alignment later anyway, if it turns out to be relevant. Maybe rename the version that returns the alignment out?

Passing a constant in，which has the alignment of the constant，return a constantpool，which need to specify the alignment of constantpool. We cannot query the alignment of constantpool now as the result could be cp + offset.

I think you're missing my point. If the caller has a particular idea of what alignment it needs, it will request it. The result will have at least that alignment. If the actual alignment comes out higher, it doesn't really matter.

In D91053#2913869, @shchenz wrote:

In D91053#2912214, @qiucf wrote:

In D91053#2912191, @lkail wrote:

A few months ago, steven shared his patch to us. For me, it should be an optimization of TOC layout, it looks odd to me to get it done at isel phase. I think we should figure out a new approach for this. What do you think @nemanjai @qiucf ?

I tend to not pursue this patch and might have a new one to get TOC layout optimization done.

I think it's located at DAG because we lower such constant pools stuff there. If this can be target independent (as Eli commented in previous patch) in DAG, or put it after ISel (I guess you mean that?), it's fine to abandon that.

I don't think after ISEL is a good idea. After ISel, the constant pool and the code pattern to access the constant pool are both settled down. It does not make sense to me to revert or change them later. It is better to generate it like we expect in the first place.

+1

In D91050#2905380, @efriedma wrote:

Passing an alignment in and returning an alignment out at the same time seems unlikely to be useful. DAGCombine can refine the alignment later anyway, if it turns out to be relevant. Maybe rename the version that returns the alignment out?

Also, if the point is literally just "glue a bunch of ConstantPools together", can we teach target-independent code to do that? It doesn't actually require anything target-specific, I think, and it should be useful on any target where computing the address of a constant pool isn't cheap.

In D106431#2907688, @whisperity wrote:

In D106431#2907002, @Sockke wrote:

Any thoughts? : )

First, let's first fix that we should still warn for the uninitialised enum case, without a FixIt. That's the issue at hand, right now, Clang-Tidy generates, as you identified, broken output. We can discuss the later steps after this is fixed. Please implement this logic, and update the patch, so we have a snapshot of how that would look like and the thing working.

+1

In D106431#2892866, @MTC wrote:

In D106431#2892859, @whisperity wrote:

Is this the right decision to make, conceptually? It will leave the variable uninitialised still, and reading such an uninit variable is still an issue, even if it is an enum.

Yeah, that's right. However, it's much more difficult to give enum an initial value than an integer.

Could we consider the alternative of warning the user about the uninitialized variable, just not offering an automatic (and potentially bad / incorrect) fix?

Make sense, we (ByteDance) are also hesitating whether we should provide automatic repair for uninitialized variables, because automatic fix may change the program semantics.

In D100162#2682340, @djtodoro wrote:

Does this solve the problem?

LGTM. But please do a complete test before enabling it by default.

Hi, @djtodoro, do you have any concern on this ? If not, I will land this patch days later.

Sorry, I don't mean to accept as PowerPC now ...

LGTM. But please hold on for some days to see if @nemanjai has more comments.

LGTM now. But please hold on for at least one week to see if @nemanjai has concern.

It seems that, craig is working on this bugzallia issue. Thank you for reporting this.

The bif/bic difference seems logically fine and probably better overall since we're using less registers, but someone who knows AArch should comment.
Also, it would be interesting to know why the code changed because we are seemingly producing the same set of SDNode ops? There might be another combine/lowering opportunity.

Split the BR_CC into another patch which will enhance the legalizer to support it. For SETCC/SELECT_CC, we will do it inside PowerPC as what other target did.

Address comments.

LGTM.

In D92083#2504734, @nemanjai wrote:

The description isn't really adequate - it does not answer my question at all really. The question is why not set the legalization action for BR_CC as Expand? Namely, what does the legalizer do that we don't want it to do if we let it expand the node? You just answered the question with the equivalent of "The legalizer does its job and that's not what we want." So please explain why that is not what we want.

Gentle ping...

@nemanjai Do you have any more comments on this ? Thank you!

Ping...

Thank you for doing this. LGTM now.

This is the same as what I want to do in D86681. As D86684 is still pending, it makes sense to split from it now.

LGTM. Please hold on for days to see if @nemanjai has more comments.

Rebase the patch.

Ping ...

LGTM, but please hold on for days to see if @craig.topper or @nemanjai have more comments.

It surprises to me that you didn't see the difference between LE and AIX, which doesn't make sense to me. Please double confirm to see if there is any conflict when update the test with the script.

Gentle ping ...

Hmm, so we can try to remove those parts that not relative with the parameter arsenm mentioned. I guess most of the check in the DAGCombiner could be removed if I understand correctly. To remove it completely, some work is needed for parameters or others that current IR cannot represent the semantics of the global setting. Is it right ?

• Reason why we need to change the interface

Ping...