Page MenuHomePhabricator

Change the error message when `-split-input-file` is used with mlir-opt to make it recognizable by IDEs
ClosedPublic

Authored by mehdi_amini on Dec 15 2020, 8:44 PM.

Details

Summary

By adding the line number of the split point immediately after the file
name (separated by :) this is recognized by various tool as a proper
location.

Ideally we would want to point to the line of the error, but that would
require some very invasive changes I suspect.

Diff Detail

Event Timeline

mehdi_amini created this revision.Dec 15 2020, 8:44 PM
mehdi_amini requested review of this revision.Dec 15 2020, 8:44 PM

Could you add an example output? I'm trying to think what it would look like (I was wondering yesterday hoe invasive it would be, if the input could be just an offset into the main buffer but I didn't do any digging ... May be something needed in underlying file manager to allow it)

It'll show /llvm-project/mlir/test/IR/invalid.mlir:14+:3:27: error: unexpected error: invalid tensor element type

And the IDE would jump to /llvm-project/mlir/test/IR/invalid.mlir:14 ? Or it would actually go to the location? I'm just thinking if a user would know this is relative offset and the location is the split being pointed to or if something like for split at /llvm-project/mlir/test/IR/invalid.mlir:14 within split :3:27 would give same IDE behavior while self-documenting?

Any update here?

And the IDE would jump to /llvm-project/mlir/test/IR/invalid.mlir:14 ?

Yes, this is the beginning of the split thought.

I'm just thinking if a user would know this is relative offset and the location is the split being pointed to or if something like for split at /llvm-project/mlir/test/IR/invalid.mlir:14 within split :3:27 would give same IDE behavior while self-documenting?

This would be nice, but I think we need a more intrusive change for that.

And the IDE would jump to /llvm-project/mlir/test/IR/invalid.mlir:14 ?

Yes, this is the beginning of the split thought.

I'm just thinking if a user would know this is relative offset and the location is the split being pointed to or if something like for split at /llvm-project/mlir/test/IR/invalid.mlir:14 within split :3:27 would give same IDE behavior while self-documenting?

This would be nice, but I think we need a more intrusive change for that.

Would it? Doesn't my suggestion just require changing + to within split ?

I'm just thinking if a user would know this is relative offset and the location is the split being pointed to or if something like for split at /llvm-project/mlir/test/IR/invalid.mlir:14 within split :3:27 would give same IDE behavior while self-documenting?

This would be nice, but I think we need a more intrusive change for that.

Would it? Doesn't my suggestion just require changing + to within split ?

I am not sure we're reading it the same way.
Assuming an error happens at line 17 in the file, for a split at line 14 (and column 27), with this change we'd display: /llvm-project/mlir/test/IR/invalid.mlir:14+:3:27: error:.
What you suggest would be /llvm-project/mlir/test/IR/invalid.mlir:14 within split 3:27: error:, what I'd like ideally would be /llvm-project/mlir/test/IR/invalid.mlir:17:27 within split at 14: error:
I.e. I'd expect within split to point at the split point, not introduce the offset within the split.

jpienaar accepted this revision.Feb 8 2021, 7:10 AM

What you suggest would be /llvm-project/mlir/test/IR/invalid.mlir:14 within split 3:27: error:, what I'd like ideally would be /llvm-project/mlir/test/IR/invalid.mlir:17:27 within split at 14: error:

Exactly (well I had a prefix to the filename too, but beyond that) and I agree with the ideal even though I'd be happy with just /llvm-project/mlir/test/IR/invalid.mlir:17:27: error:

Assuming an error happens at line 17 in the file, for a split at line 14 (and column 27), with this change we'd display: /llvm-project/mlir/test/IR/invalid.mlir:14+:3:27: error:

My initial point was that + is a bit magical there - if I got file:14+:3:27 I would need to look it up (esp with : before 3). So something that is locally documented (even if verbose) seemed preferred as this is probably not common

post split at llvm-project/mlir/test/IR/invalid.mlir:14 with offset:3:27: error:
This revision is now accepted and ready to land.Feb 8 2021, 7:10 AM

Tweak the wording/format to be more verbose/self-explanatory

jpienaar accepted this revision.Feb 26 2021, 3:31 PM