[flang-commits] [PATCH] D149442: [flang][hlfir] Add hlfir.region_assign and its hlfir.yield terminator

[Jean Perier via Phabricator via flang-commits]

jeanPerier added inline comments.


================
Comment at: flang/include/flang/Optimizer/HLFIR/HLFIROps.td:838-844
+    Example: "X = Y",  where "=" is a user defined elemental assignment "foo"
+    taking Y by value.
+    ```
+    hlfir.region_assign {
+      hlfir.yield %y : !fir.box<!fir.array<?x!f32>>
+    } to {
+      hlfir.yield %x : !fir.box<!fir.array<?x!fir.type<t>>>
----------------
tblah wrote:
> I think maybe something like
> 
> hlfir.region_assign {
>    hlfir.yield %x ...
> } = {
>   hlfir.yield %y ...
> }
> 
> Would be easier. I know there is already precedent (fir.store) for "RHS to LHS" but when I read "assign" I will be already thinking "LHS = RHS".
> 
> This is a matter of opinion so feel free to continue without changing if you prefer "RHS to LHS".
I do not have a strong opination either. If it was not for the fir.store / hlfir.assign / llvm store precedent, I would probably have gone LHS "something" RHS.  I am not sure why LLVM store chose to put the value as its first operand.

I tried your suggestion, and unfortunately it seems "=" is a reserved keyword in MLIR parser and cannot be used here. I get the funny error:
````
region_assign.fir:15:5: error: custom op 'hlfir.region_assign' expected '='
  } = {
    ^
``` 

I find "equal" too predicate like, maybe "with" could do, also I find "assign y to x" a bit more natural than "assign x with y" (but I am not a native English speaker).
 
Do you see good short word I could use instead of "="? 


================
Comment at: flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp:1046
+
+static mlir::ParseResult parseYieldOpCleanup(mlir::OpAsmParser &parser,
+                                             mlir::Region &cleanup) {
----------------
tblah wrote:
> I think this and printYieldOpCleanup are unused. Are they for something added in a later patch?
They are used in code generated by MLIR because the .td inline assembly format for YieldOp uses `custom<YieldOpCleanup>($cleanup)`.

See the custom parsing directives https://mlir.llvm.org/docs/DefiningDialects/Operations/#custom-directives

I try to use this when possible instead of defining a complete cpp print/parse implementation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149442