This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Arith] Fold trunci with ext if the bit width of the input type of ext is greater than the bit width of the result type of trunci
ClosedPublic

Authored by Miss_Grape on Dec 22 2022, 8:32 PM.

Details

Summary

This patch is mainly to deal with folding trunci with ext,as flows:

trunci(zexti(a)) -> trunci(a)
trunci(sexti(a)) -> trunci(a)

Diff Detail

Event Timeline

Miss_Grape created this revision.Dec 22 2022, 8:32 PM
Miss_Grape requested review of this revision.Dec 22 2022, 8:32 PM

Please provide a commit description that explains what you do and, most importantly, why -- https://mlir.llvm.org/getting_started/Contributing/#commit-messages.

Also naming nit: the code you are editing is a folding, not a canonicalization.

ftynse added inline comments.Dec 28 2022, 1:29 AM
mlir/test/Dialect/Arith/canonicalize.mlir
432

Nit: looking at the name, I'd expect to see trunc(ext(trunc)) in the body.

Miss_Grape retitled this revision from [MLIR][Arith] Canonicalize trunci with ext to [MLIR][Arith] Fold trunci with ext.Dec 29 2022, 7:33 PM
Miss_Grape edited the summary of this revision. (Show Details)
Miss_Grape edited the summary of this revision. (Show Details)Dec 29 2022, 7:46 PM

address the comment

Miss_Grape marked an inline comment as done.Dec 29 2022, 8:08 PM
ftynse requested changes to this revision.Dec 30 2022, 1:08 AM

The commit description still doesn't describe the details of what it is doing and why, only confuses the reader. The code before this commit *was folding trunc(ext)"! This commit does *not* add that. It does, however, *fix* a case that was mishandled. Please find the appropriate way to express that.

mlir/lib/Dialect/Arith/IR/ArithOps.cpp
1245–1259

The following

Type dstType = getElementTypeOrSelf(getType());
Value src = getOperand().getDefiningOp()->getOperand(0);
Type srcType = src.getType();
if (srcType.cast<IntegerType>().getWIdth() >
    dstType.cast<IntegerType>().getWidth()) {
  setOperand(src);
  return getResult();
}
return src;

looks more readable, concise and avoids doing the same thing twice using different APIs (querying the kind of the defining op, getting its operand).

This revision now requires changes to proceed.Dec 30 2022, 1:08 AM
Miss_Grape updated this revision to Diff 485953.Jan 3 2023, 3:50 AM

address the comment

Miss_Grape marked an inline comment as done.Jan 3 2023, 3:51 AM

The actual comment was not addressed:

The commit description still doesn't describe the details of what it is doing and why, only confuses the reader. The code before this commit *was folding trunc(ext)"! This commit does *not* add that. It does, however, *fix* a case that was mishandled. Please find the appropriate way to express that.

Miss_Grape retitled this revision from [MLIR][Arith] Fold trunci with ext to [MLIR][Arith] Fold trunci with ext if the bit width of the input type of ext is greater than the bit width of the result type of trunci.Jan 26 2023, 8:28 PM
Miss_Grape edited the summary of this revision. (Show Details)
Miss_Grape edited the summary of this revision. (Show Details)Jan 26 2023, 8:32 PM
Miss_Grape edited the summary of this revision. (Show Details)

The actual comment was not addressed:

The commit description still doesn't describe the details of what it is doing and why, only confuses the reader. The code before this commit *was folding trunc(ext)"! This commit does *not* add that. It does, however, *fix* a case that was mishandled. Please find the appropriate way to express that.

address the comments

ftynse accepted this revision.Jan 27 2023, 2:52 AM
This revision is now accepted and ready to land.Jan 27 2023, 2:52 AM