[PATCH] D130956: [X86][MC] Always emit `rep` prefix for `bsf`

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/test/CodeGen/X86/peephole-na-phys-copy-folding.ll:376
   %cmp = icmp sgt i32 %val, 0
   %res = tail call i32 asm "bsfl $1,$0", "=r,r,~{cc},~{dirflag},~{fpsr},~{flags}"(i32 %val)
   store i32 %res, ptr %mem, align 4
----------------
craig.topper wrote:
> skan wrote:
> > craig.topper wrote:
> > > skan wrote:
> > > > aaronpuchert wrote:
> > > > > pengfei wrote:
> > > > > > aaronpuchert wrote:
> > > > > > > Goes a bit too far I think. We can turn a generic builtin into `rep; bsf`, but if the inline assembly explicitly asks for `bsf` I think we should emit that.
> > > > > > You are right. So this is to check *no* REP prefix was generated. :)
> > > > > Sorry, I missed the `-NOT`. But is that needed? After all the `-NEXT` shouldn't match if there is a `rep` in between.
> > > > > 
> > > > > If there is a reason to keep this, the second occurrence should likely be `CHECK64` instead of `CHECK32`.
> > > > I think the two `CHECK-NOT` are redundant here b/c `CHECK-NEXT` can gurantee `#APP` is followed by `bsfl`.
> > > Doesn't the CHECK-NEXT only guarantee that the next line contains bsfl. It doesn't rule out any text before the bsfl.
> > I think if there is `rep` between `#APP` and `#NO_APP`, the following check will fail.
> > ```
> > ; CHECK32-NEXT:    #APP
> > ; CHECK32-NEXT:    bsfl %edx, %edx
> > ; CHECK32-NEXT:    #NO_APP
> > ```
> The test still passes with this change
> 
> ```
> diff --git a/llvm/test/CodeGen/X86/peephole-na-phys-copy-folding.ll b/llvm/test/CodeGen/X86/peephole-na-phys-copy-folding.ll
> index f3d4b6221d08..4c7094d5c5f0 100644
> --- a/llvm/test/CodeGen/X86/peephole-na-phys-copy-folding.ll
> +++ b/llvm/test/CodeGen/X86/peephole-na-phys-copy-folding.ll
> @@ -371,7 +371,7 @@ define i1 @asm_clobbering_flags(ptr %mem) nounwind {
>  entry:
>    %val = load i32, ptr %mem, align 4
>    %cmp = icmp sgt i32 %val, 0
> -  %res = tail call i32 asm "bsfl $1,$0", "=r,r,~{cc},~{dirflag},~{fpsr},~{flags}"(i32 %val)
> +  %res = tail call i32 asm "rep bsfl $1,$0", "=r,r,~{cc},~{dirflag},~{fpsr},~{flags}"(i32 %val)
>    store i32 %res, ptr %mem, align 4
>    ret i1 %cmp
>  ```
> 
> That puts rep on the same line before bsfl in the output. Every test change in this review shows rep on the same line as the bsfl.
Craig is correct. We'd better to add explicit check to avoid false negative, though I think re-generate from tool will emit the `rep` for the opposite case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130956