[PATCH] D35014: [X86] PR32755 : Improvement in CodeGen instruction selection for LEAs.

[Lama via Phabricator via llvm-commits]

lsaba added inline comments.


================
Comment at: test/CodeGen/X86/lea-opt-csebb.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+
----------------
jbhateja wrote:
> lsaba wrote:
> > can you please add a test case that covers scale >1 cases 
> 
> This commit if you see has two parts
> 1/ pattern matching based on addressing mode (which is limited currently).
> 2/ factoring of LEAs which is generic.
> 
> Checking in incremental changes should be fine I guess.
> 
> Generic pattern will need to be brought out of addessing mode based selection as I described in following link 
> https://groups.google.com/forum/#!topic/llvm-dev/x2LDXpON500
> 
> Please comment in the thread. 
> 
I am not sure i understand what you mean by "Generic pattern will need to be brought out of addessing mode" , as far as i understand, for the following C code:

int foo(int a, int b) {
  int x = a + 2*b + 4; 
  int y = a + 4*b + 4; 
  int c = x*y ;
  return c; 
}

the currently  generated IR:
define i32 @foo(i32 %a, i32 %b) local_unnamed_addr #0 {
entry:
  %mul = shl i32 %b, 1
  %add = add i32 %a, 4
  %add1 = add i32 %add, %mul
  %mul2 = shl i32 %b, 2
  %add4 = add i32 %add, %mul2
  %mul5 = mul nsw i32 %add1, %add4
  ret i32 %mul5
}


the currently generated asm: 

	leal	4(%rdi,%rsi,2), %ecx
	leal	4(%rdi,%rsi,4), %eax
	imull	%ecx, %eax
	retq

this will be refactored by this optimization in this current commit (not a future commit) to: 
	leal	4(%rdi,%rsi,2), %ecx
	leal	 (%ecx,%rsi,2), %eax
	imull	%ecx, %eax
	retq


please correct me if im wrong




================
Comment at: test/CodeGen/X86/lea-opt-cst.ll:3
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
RKSimon wrote:
> Drop the triple/datalayouts and just use the -mtriple=x86_64-unknown-linux-gnu instead.
> 
> Add a i686-unknown-linux-gnu test as well. 
> 
> By convention we tend to use X86 as the prefix for i686 triples and X64 for x86_64 triples.
> 
> You should be able to use utils/update_llc_test_checks.py to generate the codegen.
please generate the test with the original checks before your changes and commit it first in a separate commit


================
Comment at: test/CodeGen/X86/umul-with-overflow.ll:6
 define zeroext i1 @a(i32 %x)  nounwind {
+; CHECK-LABEL: a:
+; CHECK:       # BB#0:
----------------
jbhateja wrote:
> lsaba wrote:
> > why did this test change? 
> Beecause I generated its output with script utils/update_llc_test_checks.py which adds 
> an assertion for each instruction. I think it sould be fine. 
This needs to be in a separate pre commit. please commit and rebase 


https://reviews.llvm.org/D35014