[PATCH] D29014: [SelDag] Add FREEZE

Tue Mar 24 08:34:46 PDT 2020

aqjune marked an inline comment as done.
aqjune added inline comments.

================
Comment at: llvm/test/CodeGen/X86/freeze-legalize.ll:8-12
+; CHECK-NEXT:    movl $303174162, %eax ## imm = 0x12121212
+; CHECK-NEXT:    movl $875836468, %ecx ## imm = 0x34343434
+; CHECK-NEXT:    movl $1448498774, %edx ## imm = 0x56565656
+; CHECK-NEXT:    xorl %eax, %edx
+; CHECK-NEXT:    movl $2021161080, %eax ## imm = 0x78787878
----------------
spatel wrote:
> aqjune wrote:
> > spatel wrote:
> > > aqjune wrote:
> > > > aqjune wrote:
> > > > > spatel wrote:
> > > > > > Do we need to add basic simplify / constant folding for SDAG ?
> > > > > > freeze ( Constant ) --> Constant
> > > > > Yes, I agree it will be great. Do you want to make this patch contain the change as well?
> > > > Or I can land this first, and add the simplify / constant folding for SDAG.
> > > > I prefer incrementally making things because this patch itself is a big change.
> > > I prefer smaller patches too. Let's make that a follow-up. 
> > > 
> > > Is it correct that there is very little chance that this patch will create a visible performance regression (because there should be almost no freeze instruction creation in IR yet)?
> > Yes, it is.
> > Currently there is only one place where freeze is introduced - https://reviews.llvm.org/D76179
> > I checked that from assembly outputs of LLVM test-suite , only 3 / 5239 files are affected by this patch.
> That sounds good then. But can we avoid those 3 regressions cases before or within this patch? Ideally, we don't want to knowingly regress anything.
Among 3 files, two were simple regressions that had a bit more verbose assembly:
```
...
sete  %al    
orb %bpl, %al
jne .LBB9_1  
=>
...
sete  %al    
orb %bpl, %al
testb $1, %al
jne .LBB9_1  
```

```
...
testb $1, %al
je  .LBB0_2  
=>
...
movl  %eax, %ecx 
andl  $1, %ecx   
je  .LBB0_2
```

Case 3's assembly diff was bigger, so needs inspection. I can visit it after the simpler two cases are resolved.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D29014/new/

https://reviews.llvm.org/D29014