[PATCH] D107082: [X86][RFC] Enable `_Float16` type support on X86 following the psABI

[Jean Perier via Phabricator via cfe-commits]

jeanPerier added a comment.

Hi @pengfei, I am working on flang, and after this patch, we started to see 
some bugs in Fortran programs using REAL(2) (which is fp16 in flang). I am not 
an expert in LLVM codegen and the builtins, but I am wondering if there is not 
issue with how llvm codegen thinks `__truncsfhf2` returns its value and how the 
runtime actually does return it.

Here is an llvm IR reproducer for a bug we saw:

  define void @bug(ptr %addr, i32 %i) {
%1 = sitofp i32 %i to half
store half %1, ptr %addr, align 2
ret void
  }

After this patch the generated assembly on X86 is:

  bug:# @bug
  pushrbx
  mov rbx, rdi
  cvtsi2ssxmm0, esi
  call__truncsfhf2@PLT
  pextrw  eax, xmm0, 0
  mov word ptr [rbx], ax
  pop rbx
  ret

When running this from a C program to test integers are casted to floats, I am 
only seeing the bytes of the passed address being set to zero (regardless of 
the input). It seems to me that there is an issue around the `__truncsfhf2` 
interface. The `pextrw  eax, xmm0, 0` after the call seems to suggest LLVM 
codegen is looking for the result in xmm0 register, but it seems that 
`__truncsfhf2` is only returning it in eax.

Do you have any idea what could be the issue ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107082

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107082: [X86][RFC] Enable `_Float16` type support on X86 following the psABI

[Jean Perier via Phabricator via cfe-commits]

jeanPerier added a comment.

In D107082#3632301 , @pengfei wrote:

> Hi @jeanPerier , yes, you are right. This patch changes the calling 
> conversion of fp16 from GPRs to XMMs. So you need to update the runtime. If 
> you are using compiler-rt, you could simply re-build it with trunk code, or 
> at least after rGabeeae57 
> . If you 
> are using your own runtime, you can solve the problem through the way in 
> https://github.com/llvm/llvm-project/issues/56156

Thanks for the quick reply.  I was using a compiler-rt from the trunk source 
but not building it with a clang compiler compiled from the trunk. I did not 
know the version of clang used to compiled compiler-rt mattered that much. 
Using clang from the trunk (or at least after the commit you mentionnned) 
solved my problem. Thanks !


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107082

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Jean Perier via Phabricator via cfe-commits]

jeanPerier accepted this revision.
jeanPerier added a comment.
This revision is now accepted and ready to land.

The lowering part looks good to me (I only have a minor comment inlined about a 
header used in lowering).




Comment at: flang/include/flang/Runtime/environment-defaults.h:19
+  std::string defaultValue;
+};
+

I think this header may better belong to Semantics (or even Lower since it is 
only used there) in the sense that it does not define a data structure that is 
used at runtime, but it defines a data structure so that we can keep track of 
some default environment value during compilation (It is not a huge deal, but I 
am a little bit wary of seeing std::string in Fortran::runtime while the 
runtime is meant to be free of the C++ runtime).


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Jean Perier via Phabricator via cfe-commits]

jeanPerier accepted this revision.
jeanPerier added a comment.

In D130513#3832241 , @jpenix-quic 
wrote:

> Thank you @jeanPerier for looking over the lowering portion! Regarding moving 
> the header (I'm replying to the comment here since the inline one now opens 
> in a separate revision/window as the file is gone), I moved it to 
> Lower/EnvironmentDefault.h as lowering is where I use it (as you mentioned). 
> Please let me know if this needs further changes!

LGTM, thanks.


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140415: [flang] stack arrays pass

[Jean Perier via Phabricator via cfe-commits]

jeanPerier added a comment.

Thanks for all the updates. This looks functionally correct to me. Since I am 
not very familiar with this kind of analysis and transformation, it would be 
great if another reviewer could give his/her opinion. But otherwise, given this 
solution is well isolated from a code point of view and can be turned and 
on/off easily, I'll be glad to approve it.




Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:352
+  LLVM_DEBUG(llvm::dbgs()
+ << "--Allocation is not for an array: skipping\n");
+}

I think the early return may be missing here.



Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:446
+  candidateOps.insert({allocmem, insertionPoint});
+}
+  }

nit: MLIR/LLVM coding style do not use `{}` for single line if.



Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:642
+rewriter.setInsertionPointAfter(op);
+  else if (mlir::Block *block = insertionPoint.tryGetBlock())
+rewriter.setInsertionPointToStart(block);

If this case must succeed when the other failed, it may be better to place it 
in an `else {` and assert that a block was obtained, so that it is certain that 
the insertion point was correctly set when looking at this code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140415

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140415: [flang] stack arrays pass

[Jean Perier via Phabricator via cfe-commits]

jeanPerier added inline comments.
Herald added a subscriber: sunshaoce.



Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:104
+
+  bool operator!=(const InsertionPoint &rhs) const {
+return (location != rhs.location) ||

It's better to negate the `== operator` here so that the implementation logic 
cannot diverge.



Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:120
+  // which operations we intend to rewrite before it calls the pattern rewriter
+  llvm::SmallDenseMap insertionPoints;
+

It is definitely weird to me to have this in the lattice points. It seems 
expensive to save this at every program point and to compute it every time a 
state changes on a maybe not final candiate.

Why not computing in StackArraysAnalysisWrapper::analyseFunction on the final 
candidates (the value in stateMap at that are freed on all return paths) ?



Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:272
+// it gets freed by the time we return
+return AllocationState::Allocated;
+  }

This is still odd to me because this breaks the monocity requirement of join:

`join(join(freed, unknown), allocated) ) = join(unknown, allocated) = allocated`

while

`join(freed, join(unknown, allocated)) = join(freed, allocated) = unknown`

I still do not think you need anything special here given the fact that an 
allocation done on a path is considered in the end already seems accounted for 
in LatticePoint::join since the state is added even if not present in the other 
latice.



Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:333
+  stateMap[value] = state;
+  addInsertionPoint(value, state);
+  return mlir::ChangeResult::Change;

As mentioned in my other comment above, I do not get why the insertion point is 
computed at that point while it seems the analysis (after computing the states, 
and using the lattice state at the func.return) is not over for the function (I 
would expect insertion to be computed only for the successfully identified 
allocmem at the end, not the one that may be candidate on one code path).



Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:460
+  lattice->appendFreedValues(candidateOps);
+}
+  });

I think this is not correct: It seems this will consider every FreememOp that 
could be paired with an allocmem as candidate:

```
func.func @test(%cdt: i1) {
  %a = fir.allocmem !fir.array<1xi8>
  scf.if %cdt {
fir.freemem %a : !fir.heap>
  } else {
  }
  return
}
```

Why not considering the func.return lattice states instead ?

Note that it seems fir.if is not considered a branch operation, so the state of 
its blocks are reset on entry. That is why scf.if is needed to create a test 
exposing the issue. Maybe fir.if op should be given the right interface, but 
that is another topic.



Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:509
+
+static bool isInLoop(mlir::Block *block) { return mlir::blockIsInLoop(block); }
+

Where is `blockIsInLoop` defined ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140415

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Jean Perier via Phabricator via cfe-commits]

jeanPerier added a comment.

@agozillon, in the test added here (omp-frontend-forwarding.f90), I am seeing 
failures in some patches windows pre-merge checks that I think are not related 
to the patches.
Could you check if there is a stability/reproducibility issue with the 
omp-frontend-forwarding.f90 on windows?

The failure message look like:

  # command stderr:
  
C:\ws\w8\llvm-project\premerge-checks\flang\test\Driver\omp-frontend-forwarding.f90:21:23:
 error: CHECK-OPENMP-EMBED: expected string not found in input
  ! CHECK-OPENMP-EMBED: "{{[^"]*}}clang-offload-packager" {{.*}} 
"--image=file={{.*}}.bc,triple=amdgcn-amd-amdhsa,arch=gfx90a,kind=openmp"
^
  :6:435: note: scanning from here
   "c:\\ws\\w8\\llvm-project\\premerge-checks\\build\\bin\\flang-new" "-fc1" 
"-triple" "amdgcn-amd-amdhsa" "-emit-llvm-bc" "-fopenmp" "-mrelocation-model" 
"pic" "-pic-level" "2" "-fopenmp-is-device" "-o" 
"C:\\Users\\ContainerAdministrator\\AppData\\Local\\Temp\\lit-tmp-1koflt_t\\omp-frontend-forwarding-gfx90a-6808f0.bc"
 "-x" "f95-cpp-input" 
"C:\\ws\\w8\\llvm-project\\premerge-checks\\flang\\test\\Driver\\omp-frontend-forwarding.f90"





^
  :7:266: note: possible intended match here
   "c:\\program files\\llvm\\bin\\clang-offload-packager.exe" "-o" 
"C:\\Users\\ContainerAdministrator\\AppData\\Local\\Temp\\lit-tmp-1koflt_t\\omp-frontend-forwarding-9beea6.out"
 
"--image=file=C:\\Users\\ContainerAdministrator\\AppData\\Local\\Temp\\lit-tmp-1koflt_t\\omp-frontend-forwarding-gfx90a-6808f0.bc,triple=amdgcn-amd-amdhsa,arch=gfx90a,kind=openmp"

I wonder if this is an issue with the ".exe" command suffix in the windows 
output.

Example of pre-merge failures:

https://reviews.llvm.org/D146989
https://buildkite.com/llvm-project/premerge-checks/builds/143821#01872b65-9b0c-457b-8714-c1f0ca00d02b

or
https://reviews.llvm.org/D147130
https://buildkite.com/llvm-project/premerge-checks/builds/143873#01872ccb-9251-499a-b9fd-9155e3ffb1f1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145815

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Jean Perier via Phabricator via cfe-commits]

jeanPerier added a comment.



> Hi @jeanPerier,
>
> Thank you! I noticed this today as well and I'm currently looking into it, i 
> have a fix upcoming, just forcing another patch 
> (https://reviews.llvm.org/D144896) to test it via buildbot as I have no easy 
> access to a windows machine to test it at the moment, and yes @jhuber6 and 
> you are correct I believe, it appears to be the .exe suffix on windows! I'll 
> submit the fix as soon as it clears the buildbot.

Thanks a lot for fixing the issue!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145815

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151088: [flang][hlfir] Separate -emit-fir and -emit-hlfir

[Jean Perier via Phabricator via cfe-commits]

jeanPerier accepted this revision.
jeanPerier added a comment.
This revision is now accepted and ready to land.

Not the driver expert here, so please wait for @awarzynski input, but the new 
flag logic LGTM from a user point of view.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151088

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140415: [flang] stack arrays pass

[Jean Perier via Phabricator via cfe-commits]

jeanPerier accepted this revision.
jeanPerier added a comment.

I do not have any further comments, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140415

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits