Updated full patch to solve "content not available" error.

Separate review created for test cases. (https://reviews.llvm.org/D88551)

In D86395#2274541, @lebedev.ri wrote:

In D86395#2274522, @Jac1494 wrote:

In D86395#2269436, @spatel wrote:

In D86395#2268634, @Jac1494 wrote:

test53 and test55 has been removed.

We are going in circles. I'll give this 1 more try, and if it still doesn't make sense, then maybe another reviewer can better communicate what we expect for the tests:

1. There are 4 commutative patterns variations in the code.
2. Each one of those patterns should have a test (we should have at least 4 tests).
3. Each test corresponds to exactly one of the commutative patterns.
4. Each test should be in canonical form without this patch (the baseline CHECK lines should correspond exactly to the IR as written).
5. The 8 step check I provided earlier may be used to confirm that the tests provide the expected coverage for the code.

@spatel, apologies for the confusion created due to multiple revisions.

Let's consider the initial pattern and associated commutative version of this.

initial pattern:-
(~A & B) ^ A -> (A | B) 

And 8 possible commutative version of this.
1) (~A & B) ^ A -> (A | B) 
2) (~B & A) ^ B -> (A | B)
3) (B & ~A) ^ A -> (A | B)  --> identical to 1)
4) A ^ (~A & B) -> (A | B)  --> identical to 1)
5) A ^ (B & ~A) -> (A | B)  --> identical to 1)
6) (A & ~B) ^ B -> (A | B)  --> identical to 2)
7) B ^ (~B & A) -> (A | B)  --> identical to 2)
8) B ^ (A & ~B) -> (A | B)  --> identical to 2)

As you may notice that all these patterns (IR) are reducing to 2 unique patterns (pattern 1 and pattern 2).
So pattern 1 and 2 are sufficient for the coverage of all commutative variations of the primary pattern.

How so?
If you only have a test for pattern 1 and pattern 2, how do you know that the commutative variants are handled?

Does this matches with you understandings ??

In D86395#2269436, @spatel wrote:

In D86395#2268634, @Jac1494 wrote:

test53 and test55 has been removed.

We are going in circles. I'll give this 1 more try, and if it still doesn't make sense, then maybe another reviewer can better communicate what we expect for the tests:

1. There are 4 commutative patterns variations in the code.
2. Each one of those patterns should have a test (we should have at least 4 tests).
3. Each test corresponds to exactly one of the commutative patterns.
4. Each test should be in canonical form without this patch (the baseline CHECK lines should correspond exactly to the IR as written).
5. The 8 step check I provided earlier may be used to confirm that the tests provide the expected coverage for the code.

Gentle Ping !!

test53 and test55 has been removed.

In D86395#2268209, @spatel wrote:

In D86395#2268155, @Jac1494 wrote:

In D86395#2268096, @spatel wrote:

In D86395#2260871, @spatel wrote:

Try this experiment to see if your tests provide the coverage that we expect:

1. Replace the "m_c_XXX" matchers with the non-commute equivalent.
2. Exactly 1 test should be modified.
3. Add back only 1 of the commutative matchers.
4. Exactly 2 tests should be modified.
5. Invert the commutativity of the matchers (so the opposite matcher is now commutative).
6. Exactly 2 tests should be modified.
7. Make both matchers commutative.
8. All 4 tests should be modified.

Did you try this with the tests as shown in this draft of the patch?

Yes, @spatel I have tried this cases with tests of this patch.

Then I think something is wrong with your build environment. If I remove the commutative matchers, I get 0 test diffs.

@spatel it is also same for me. If I remove both the commutative matchers than I also get 0 test diffs.(change x_c_Xor with x_Xor and x_c_And with x_And)

In D86395#2268096, @spatel wrote:

In D86395#2260871, @spatel wrote:

Try this experiment to see if your tests provide the coverage that we expect:

1. Replace the "m_c_XXX" matchers with the non-commute equivalent.
2. Exactly 1 test should be modified.
3. Add back only 1 of the commutative matchers.
4. Exactly 2 tests should be modified.
5. Invert the commutativity of the matchers (so the opposite matcher is now commutative).
6. Exactly 2 tests should be modified.
7. Make both matchers commutative.
8. All 4 tests should be modified.

Did you try this with the tests as shown in this draft of the patch?

Addressed the review comments of @spatel and @xbolva00 .

Test cases are tested and updated.

@spatel test cases are updated as per your review comments. Thanks for this.
And these test cases were failing without this patch. So it means that patch is doing correct transformations.

Hi All,

In D86395#2246162, @spatel wrote:

If my earlier comment about the tests did not make sense, please grep for "thwart" in this test directory to see examples.

Removed unnecessary code.

Removed unnecessary code.

Addressed @xbolva00 review comments.

In D86395#2242393, @spatel wrote:

You are proposing to transform 4 commuted variations of a pattern, but your tests do not provide coverage for those variants. My test suggestion would hopefully provide that coverage.
This might be clearer if you create the tests before this code patch and step through the transforms in a debugger. Look for InstCombinerImpl::SimplifyAssociativeOrCommutative() and getComplexity().

New pattern "~(A | B) | (A ^ B) -> ~(A & B)" and Test cases are added.

In D86395#2234640, @spatel wrote:

The tests should be pre-committed with current CHECK lines (ie, without this patch), so we just see the diffs - and can confirm that the tests actually represent the patterns shown in the test comments.

Added new pattern "A ^ (~A & B) --> (A | B)" .

In D83468#2236798, @aemerson wrote:

In D83468#2236637, @Jac1494 wrote:

void Instruction::updateLocationAfterHoist() {

const DebugLoc &DL = getDebugLoc();
if (!DL)
  return;

Code Snippet from link (https://reviews.llvm.org/D85670) is not setting the line number at all.
And as seen/observed ,we can also prevent jumpy behavior by skipping the line number(By not setting to line number zero).

I'm sharing the code snippet to avoid unnecessary revision hindering the actual discussion.Is this Okey ? Or should I revise ?

That snippet is just returning if there was no debug info at all, there's a ! in the if condition.

Sorry for misunderstanding , I am also trying to address no debug location.
snippet shared above suggest the removal of this snippet from IRTransaltor.cpp

// We only emit constants into the entry block from here. To prevent jumpy
 // debug behaviour set the line to 0.
 if (const DebugLoc &DL = Inst.getDebugLoc())
   EntryBuilder->setDebugLoc(
       DebugLoc::get(0, 0, DL.getScope(), DL.getInlinedAt()));
 else
   EntryBuilder->setDebugLoc(DebugLoc());

Have you ruled this out as a gdb bug/possible improvement?

Yes , Because Arm64 with selectionDAG works fine. and we can also prevent jumpy behavior by skipping the line number(By not setting to line number zero), this way we can correct Global-isel behavior too.
+ I am also writing about this behavior to gdb mailing list.

In D83468#2226884, @dblaikie wrote:

But it's possible that the right solution is to drop the location, rather than use line zero here. Perhaps this code in GlobalISel should be using updateLocationAfterHoist ( https://reviews.llvm.org/D85670 ) @vsk @aprantl

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2183

// We only emit constants into the entry block from here. To prevent jumpy
 // debug behaviour set the line to 0.
 if (const DebugLoc &DL = Inst.getDebugLoc())
   EntryBuilder->setDebugLoc(
       DebugLoc::get(0, 0, DL.getScope(), DL.getInlinedAt()));
 else
   EntryBuilder->setDebugLoc(DebugLoc());

Added more test cases.

Are you ok with this..?

Addressed @xbolva00 review comments. Thanks

Addressed @xbolva00 review comments. Thanks

Addressed @lebedev.ri review comments.Thanks

Also possible it's not a bug at all & GlobalISel just ends up doing some optimization that does end up creating a necessary/correct line 0 at the start of the function where SelectionDAG/FastISel does not.

I would consider this to be more of a bug in GDB. Line 0 has a well-defined meaning in DWARF and stands for "no source location corresponds to this code". LLDB, for example, interprets line 0 as an indicator to skip over instructions and automatically steps over them when single-stepping through a program, since line 0 is not a meaningful place to stop. It might be worth talking to the GDB developers and ask them what they think about adding a similar feature to their handling of line 0. DWARF doesn't prescribe that that's how debuggers should behave, but I think it's a reasonable interpretation.

The IRTranslator and Legalizer do not set the line number, they're supposed to just forward the line number it already has.

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2183

In D83468#2221792, @aprantl wrote:

If the problem that you want to solve is that the prologue ends at line 0, then massaging earlier passes until the right result happens to come up in one testcase is not going to be very effective. Yes, it may fix the testcase, but that won't last long, and the patch is introducing questionable behavior that will end up causing other problems.

PING??

https://github.com/llvm/llvm-project/commit/32823091c36cfa2b27b717246f15d4f12591e6f4

I think I haven't quit understood what your goal is. Are you trying to avoid having the prologue end at line 0?

Yes, Also other line 0 too which is making incorrect debugging experience.

PING?

Issue is with Global-isel , So trying to handle issue in Legalizer. Also tested debugging behavior and it is fine.

In D83468#2177491, @aprantl wrote:

Thanks for posting the line table. It appears to confirm my suspicion that this patch merely hides the issue, but it isn't correct. To be sure it may be possible to preserve the debug location in this function, but implicitly setting it to the location of the instruction before it, is not the right thing to do. It may have the desired effect in your example, but it't not generally correct.

Hi @riccibruno,
Address your review comment.Add please use
Name :- Jaydeep Chauhan
Mail id:- jaydeepchauhan1494@gmail.com
Thanks

Hi @aaron.ballman ,
Address your review comments.
Thank you for accepting this. I don't have commit access please commit this.
Thanks.

Address @aaron.ballman review comments.
I have added your test case that is passing as well.
This patch covers this scenario also.
Thanks @aaron.ballman

Addressed @riccibruno comments.Thanks

Hi @aprantl , Thank you for reply.

ping?

Issue is with Global-isel and specific to AAarch64.

Sure. Would you like me to use the email address you use on bugzilla as the commit author? If not, could you supply an address to use?

Thank you for accepting this. I don't have commit access please commit this.

ping

In D71451#1817334, @aprantl wrote:

In D71451#1816269, @Jac1494 wrote:

ping

I'd be curious to the answer to David's questions. If the size increase is because of unused extern variables coming in from libc or something then it doesn't seem worth the cost.

clang format source file added ,and removed unnecessary attribute from test case.

that still seems very surprising - this change is specifically intended to add more debug info, is it not? So do you have any ideas/theories as to why that's not showing up in the data?

Do you have any reason to believe these patches would reduce the size of the debug info? It seems like they only add more debug info, and don't remove anything - so we'd expect an increase in the size, certainly not a decrease. That means, to me, there's probably a mistake in the measurement somehow?

DW_AT_linkage_name attribute support added for global variable declaration.

I'd be curious to the answer to David's questions. If the size increase is because of unused extern variables coming in from libc or something then it doesn't seem worth the cost.

ping

Am I reading this right that the data would suggest that enabling this feature /reduces/ the size of debug info sections? That doesn't sound right - can you explain why that would be the case? (perhaps the data is incorrect/it wasn't built with -fstandalone-debug?)

Test case Added

@Jac1494 - have you made any measurements of the size increase of this change? Perhaps a self-host build of clang?

In D71451#1786517, @dblaikie wrote:

Do you have any particular users/use case for this?

Separate clang patch with test cases .

For some real life case like below we need debuginfo for declaration of global extern variable .